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

Prevent tooltip from showing when hovering past the end of script line #100913

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

larspet
Copy link
Contributor

@larspet larspet commented Dec 29, 2024

Fixes #100767

TextEdit::get_line_column_at_pos() already had an option to not allow "out of bounds" access, but it only checked if the row was OOB, not the column. This PR renames that argument and adds an extra argument that allows for checking the column as well.

Before
tooltip_before.mp4
After
tooltip_after.mp4

@larspet larspet marked this pull request as ready for review December 29, 2024 20:36
@larspet larspet requested review from a team as code owners December 29, 2024 20:36
@Mickeon
Copy link
Contributor

Mickeon commented Dec 29, 2024

I wonder, why would one ever want the existing behaviour enough to warrant it being an optional argument?

@larspet
Copy link
Contributor Author

larspet commented Dec 29, 2024

I wonder, why would one ever want the existing behavior enough to warrant it being an optional argument?

For get_line_column_at_pos() I can see why it's needed, e.g. in order to put the caret at the last column when clicking past the end of the line. Although I think the argument is backwards, and should be named p_clamp_position or something instead.

The reason I added the argument to get_word_at_pos() was to give them similar API, and not potentially break current uses of it. That said, looking at the other few existing uses of it in the engine, this new functionality would be wanted there too. I can't speak for any user code already utilizing the function however.

If wanted, I would not be against not adding this new argument, and instead changing the current behavior of get_word_at_pos() instead.

@Chaosus Chaosus added this to the 4.4 milestone Dec 30, 2024
@dalexeev dalexeev self-requested a review January 4, 2025 14:33
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

I'm not a node GUI expert, but the code looks good to me, I tested it and it works fine.

@larspet larspet force-pushed the tooltip-hover-oob branch from 367c157 to 2dc69ed Compare January 7, 2025 17:30
@akien-mga akien-mga requested a review from bruvzg January 7, 2025 22:37
@kitbdev
Copy link
Contributor

kitbdev commented Jan 17, 2025

The behavior of get_line_column_at_pos when allow oob is false shouldn't change. A new parameter can be added to it for horizontal checking, and the existing one's name can be changed.
This is because there is code that relies on the current behavior of only checking vertical like in CodeEdit::get_cursor_shape for the fold icon, and users may be relying on it too. I would also like to use it for gutters, see #101613.

@larspet
Copy link
Contributor Author

larspet commented Jan 17, 2025

I removed the optional argument from get_word_at_pos since there really wasn't any point to it. The new behavior follows the expected behavior much more anyways.

Instead, I split the argument in get_line_column_at_pos into two (line & column). This way the old behavior is kept (so that e.g. the line-folding ... icon works), while also optionally allowing to disallow clamping for the column.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected. Code and docs look good to me.

@akien-mga akien-mga requested a review from bruvzg January 28, 2025 10:15
akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 7, 2025
Prevent tooltip from showing when hovering past the end of script line
@akien-mga akien-mga merged commit e87f4f6 into godotengine:master Feb 7, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@larspet larspet deleted the tooltip-hover-oob branch February 7, 2025 13:27
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.

Documentation tooltip trigger area includes end-of-line and whitespace surrounding a symbol
8 participants