-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix selectable label highlight offset when alignment is changed #5927
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I left a few general comments inline but I leave a more thorough review of the label code to someone with more experience in that part of the codebase.
I would like to add that you please don't remove the PR template. The checkboxes are there for a reason. Please add them back and try to tick off the first three mandatory ones. It would be good to add a test case for this to verify that the change fixes that issue. It also seems like this breaks the existing tests so that would need to be fixed as well.
| "fyne.io/fyne/v2" | ||
| "fyne.io/fyne/v2/data/binding" | ||
| "fyne.io/fyne/v2/theme" | ||
| "strings" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format the inputs correctly using goimports.
|
|
||
| l.provider.Segments[0].(*TextSegment).Style = RichTextStyle{ | ||
| Alignment: l.Alignment, | ||
| Alignment: fyne.TextAlignLeading, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this change break the set alignment for anything other than leading?
|
|
||
| // measureWrappedWidth estimates the maximum line width of `text` when rendered | ||
| // into a box of width `availW` using the given wrapping mode and style. | ||
| func measureWrappedWidth(text string, availW float32, wrap fyne.TextWrap, style fyne.TextStyle) float32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% well versed in the label and text code but I kind of feel like there already should be logic somewhere in the code to calculate things like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing selection code should have most of this maths already - it's the leading X which changes - and that should just be the existing width calculations subtracted from the width?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this only accounts for selection of a single line but multi-line text can be selected as well
Description:
Fixes the offset issue with the selectable label highlight when the label text alignment is changed.
Screenshot of the issue:
Test application to replicate the issue:
Checklist: