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

Fixed RichTextLabel wrong selection offset after drop cap #71747

Merged

Conversation

Koyper
Copy link
Contributor

@Koyper Koyper commented Jan 20, 2023

Fixes an issue in RichTextLabel where starting a selection, or selecting across a drop cap, would incorrectly start the selection offset by the width of the drop cap.

I would recommend leaving in the comments, because this is where a future improvement would fix the issue of the drop cap text not currently being selectable, and figuring out how to obtain the drop cap text is not obvious.

@Koyper Koyper requested a review from a team as a code owner January 20, 2023 17:19
@akien-mga akien-mga added this to the 4.0 milestone Jan 20, 2023
@akien-mga akien-mga requested a review from bruvzg January 20, 2023 17:25
@Koyper Koyper force-pushed the rich_text_label_dropcap_selection_bug branch from ec3b9f1 to 5b3f1dd Compare January 20, 2023 17:32
@Koyper
Copy link
Contributor Author

Koyper commented Feb 13, 2023

Here are two comparison screen videos showing before/after PR fix.

Without the fix, the selection start is offset from the mouse location by the width of the dropcap:

Screen.Recording.2023-02-13.at.8.59.56.AM.mov

With the fix, the selection is correctly linked to the mouse location:

Screen.Recording.2023-02-13.at.9.02.44.AM.mov

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 17, 2023
@YuriSizov
Copy link
Contributor

I would recommend leaving in the comments, because this is where a future improvement would fix the issue of the drop cap text not currently being selectable, and figuring out how to obtain the drop cap text is not obvious.

If this code is not valid yet, then there is no point leaving it there. You can stash the changes and work on it after we merge this. So please strip these lines from the PR.

@Koyper Koyper force-pushed the rich_text_label_dropcap_selection_bug branch from 089e26f to 175557d Compare April 11, 2023 13:52
@YuriSizov
Copy link
Contributor

You need to squash your commits.

@Koyper Koyper force-pushed the rich_text_label_dropcap_selection_bug branch from 175557d to 99376ee Compare April 11, 2023 15:10
@Koyper
Copy link
Contributor Author

Koyper commented Apr 11, 2023

If this code is not valid yet, then there is no point leaving it there. You can stash the changes and work on it after we merge this. So please strip these lines from the PR.

Is there a mechanism somewhere for adding code comments like this one for the benefit of a different, future contributor? I'll have my own comments, but it could save someone else some effort?

@YuriSizov
Copy link
Contributor

YuriSizov commented Apr 11, 2023

Is there a mechanism somewhere for adding code comments like this one for the benefit of a different, future contributor? I'll have my own comments, but it could save someone else some effort?

You can comment your suggestion on an issue which you are trying to solve. If there is no issue, you can open one and leave a comment there.

Code comments aren't discoverable and having dead code in the codebase is mostly just contributes to the noise. And you also cannot guarantee that your solution is the correct one, so leaving it in the code would be misleading.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

LGTM. bruvzg approved, commented out lines were removed.

@YuriSizov YuriSizov merged commit ca808c8 into godotengine:master Apr 11, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@Koyper
Copy link
Contributor Author

Koyper commented Apr 11, 2023

Code comments aren't discoverable and having dead code in the codebase is mostly just contributes to the noise. And you also cannot guarantee that your solution is the correct one, so leaving it in the code would be misleading.

I agree with these good observations - thanks!

@Koyper Koyper deleted the rich_text_label_dropcap_selection_bug branch April 11, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants