Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PickList text wrapping #1318

Merged
merged 5 commits into from
Apr 30, 2022
Merged

Conversation

tarkah
Copy link
Member

@tarkah tarkah commented Apr 25, 2022

PickList text is wrapped if the pick list is set to a fixed width and the selected text is wider than the fixed width. This resolves the issue by giving the rendered text bounds an infinite width so it doesn't wrap. A layer / clip needs to be provided to prevent text overflow.

I can't imagine wrapping is intended since the height is fixed to text_size and 2 lines of wrapped text inside text_size doesn't look good at all. This seems like a more sane default to handle text overflow.

@hecrj hecrj added bug Something isn't working widget labels Apr 26, 2022
@hecrj hecrj added this to the 0.4.0 milestone Apr 26, 2022
@hecrj
Copy link
Member

hecrj commented Apr 26, 2022

This was intended to some extent. Basically, I wanted to avoid an unconditional with_layer call, which breaks text caching currently.

Since a PickList is a much smaller widget than a Scrollable, there is a high chance users will use a bunch of them at once. Therefore, I considered the use of with_layer here a very potential performance hog.

We may be able to fix the issue for now by just changing the vertical_alignment to Top so the wrapped text is completely out of bounds. But of course, it will still cause the wrapped text to disappear which is not the expected behavior.

Once the text caching issues are fixed, we can revisit this!

@tarkah
Copy link
Member Author

tarkah commented Apr 27, 2022

@hecrj Makes sense w/ the performance consideration of using a new layer. I tried using Top alignment with manually centered y placement and this works, however the wrapped text still "overflows" below the picklist quad since we don't have any clipping in place.

This is actually less preferred as the wrapped text leaves the quad, whereas with Center it at least is wrapped mostly within the quad.

Let's leave this until text caching has been addressed. Thanks!

Edit: Disregard, I just needed to restrict the height of the text primitive to text size and it no longer overflows. I think this is a good intermediate solution.

@tarkah tarkah force-pushed the fix/picklist-text-wrapping branch from b9619e9 to d562c27 Compare April 27, 2022 16:24
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks 🙇

@hecrj hecrj merged commit abc9931 into iced-rs:master Apr 30, 2022
@tarkah tarkah deleted the fix/picklist-text-wrapping branch April 30, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working widget
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants