Skip to content

Added multi-line option to textbox and changed painting to match any text size#1031

Closed
rhzk wants to merge 1 commit intolinebender:masterfrom
rhzk:master
Closed

Added multi-line option to textbox and changed painting to match any text size#1031
rhzk wants to merge 1 commit intolinebender:masterfrom
rhzk:master

Conversation

@rhzk
Copy link
Collaborator

@rhzk rhzk commented Jun 13, 2020

This pull request adds a option to allow multi-line instance of textbox.
It only checks for '\n' line-breaks so '\r' alone might be a issue.

I also changed the painting of different elements so it should fit text of any size. (Old painting code had hard numbers that just happened to work with the default text size of 15.0)

I would like to have text_size as a member but I am unsure how to give it a default value as I cant access env.get() in the TextBox::new()

Shift+Enter does not trigger 'KeyCode::Return' should it be add it to that or to a new KeyCode?

@luleyleo
Copy link
Collaborator

For the text size you can probably set it to zero in the constructor and then initialize it to the actual value during LifeCycle::WidgetAdded.

@rhzk
Copy link
Collaborator Author

rhzk commented Jun 14, 2020

Thanks for the tip.

Another issue is how to check if there is a scroll widget attached and communicate with it.

Currently the focus border is drawn around the textbox when a scroll widget is attached, I need to either get the size of the scroll and draw the focus according to that size. or disable it and draw it in the scroll instead. This picture illustrates the problem (textbox grows infinite, scroll fixed size):

scroll

There is also a need to update the scroll position when people navigate downwards in the text using arrow keys. But I dont want to mess with this until we have some general idea of how those widgets should work together.

@HoNile
Copy link
Contributor

HoNile commented Jun 15, 2020

I think the correct way to count lines and handle all possible line-breaks is to use text.lines().count()

@ForLoveOfCats
Copy link
Collaborator

On the topic of widgets handling being in a scroll see #799

@rhzk
Copy link
Collaborator Author

rhzk commented Jun 16, 2020

On the documentation for text.lines().count() it says it calls next() over and over. I don't know if that gets optimized but it sounds like a lot of jump calls. And could maybe be a performance problem with large amounts of text.

It also says it counts '\n' and '\n\r', but nothing about '\r' alone. and it ignores the last line break if there is one. Which is a problem because if there is a line break on the end of the text we need to draw a new empty line, and if there is not we do not draw a new line

@HoNile
Copy link
Contributor

HoNile commented Jun 16, 2020

I have searched on the web and it look like counting only '\n' should work, specially I found this https://llogiq.github.io/2016/09/24/newline.html so if xi work by counting '\n' it's ok in all cases I guess.

Notes:

@rhzk
Copy link
Collaborator Author

rhzk commented Jun 16, 2020

That is interesting thanks, I might also set up some performance tests to see how they compare to each other.

@cmyr
Copy link
Member

cmyr commented Jun 17, 2020

My next in-the-code project is going to be a large reworking of our text API, that will include multi-line support; I'm not sure it's worth merging temporary fixes in the meantime, since there's some fairly significant work to do under the hood.

@rhzk
Copy link
Collaborator Author

rhzk commented Jun 17, 2020

This was mostly fixing the painting code so that it can draw the cursor / selection over multiple lines,
and making sure the widget draws the lines with the correct height. It might still be useful after you have rewritten the text API. Can put this on hold for now and then I can update it if/when needed to reflect the changes.

There is also the problem with scroll, so this have to wait until there is a agreed upon solution for how scrolling should be handled.

@xStrom xStrom added the S-blocked waits for another PR or dependency label Jun 20, 2020
@cmyr
Copy link
Member

cmyr commented Jul 14, 2020

going to close this for now, a solution for this is in progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-blocked waits for another PR or dependency

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants