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.

Sign up for GitHub

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

Jump to bottom

Search terms are now highlighted when the bar opens with a selection. #82707

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Search terms are now highlighted when the bar opens with a selection. #82707

merged 1 commit into from
Oct 30, 2023

Conversation

BrianMacIntosh
Copy link
Contributor

@BrianMacIntosh BrianMacIntosh commented Oct 3, 2023 •
edited by akien-mga
Loading

Previously, the code editor only updated the text highlights when a search was made, but opening the search bar with a selection does not go through that code path because the first result is already selected.

This is a fix for #82706.

Bugsquad edit:

@BrianMacIntosh BrianMacIntosh requested a review from a team as a code owner October 3, 2023 04:03
MewPurPur
MewPurPur approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@MewPurPur MewPurPur left a comment

Choose a reason for hiding this comment

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

Nitpick: dir makes me think of a directory rather than direction. Otherwise, looks excellent!

@BrianMacIntosh
Copy link
Contributor Author

Nitpick: dir makes me think of a directory rather than direction. Otherwise, looks excellent!

Thanks for the feedback - I went ahead and amended it.

@Chaosus Chaosus added this to the 4.2 milestone Oct 4, 2023
jsjtxietian
jsjtxietian reviewed Oct 6, 2023
View reviewed changes
editor/code_editor.cpp Outdated
@@ -536,6 +528,10 @@ void FindReplaceBar::_show_search(bool p_focus_replace, bool p_show_only) {
search_text->set_caret_column(search_text->get_text().length());
}

_update_flags(false);
Copy link
Contributor

@jsjtxietian jsjtxietian Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can we just add a _search_text_changed(get_search_text()) call after search_text->set_text(text_editor->get_selected_text(0)) ? Force doing a search when the search bar is shown. I did some test locally.

Copy link
Contributor Author

@BrianMacIntosh BrianMacIntosh Oct 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That would clean the code up but it triggers a search_current unnecessarily, since the the first search match is already selected in this case. I figured that's why the stuff that was already here was inlined in the first place, so I stuck with that.

If you guys think that slight inefficiency is worth the cleaner code I can change it (it doesn't affect the resulting behavior).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsjtxietian what do you think about my post above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry for the late reply. IMHO both are fine, it's a choice between a little readability and performance. I slightly perfer my suggestion because it introduces less change which could potentially introduce less bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made that change, with the exception that I left the _search_text_changed where the existing result-updating code was in the not-empty check, to avoid doing it unnecessarily and minimize the change.

@BrianMacIntosh BrianMacIntosh force-pushed the find-highlight-fix branch 2 times, most recently from 40118f1 to 4d520a2 Compare October 12, 2023 04:54
@BrianMacIntosh BrianMacIntosh requested review from a team as code owners October 16, 2023 15:34
@AThousandShips AThousandShips removed request for a team October 16, 2023 15:47
@BrianMacIntosh
Copy link
Contributor Author

Please reset your branch by doing: git reset --hard 40118f1707c5077a6de3576abc6e20f30fa7c0d8 and then try again

Thanks, I didn't realize I needed to force push after a rebase.

@BrianMacIntosh BrianMacIntosh mentioned this pull request Oct 16, 2023
Closed
@BrianMacIntosh
Search terms are now highlighted when the bar opens with a selection.
0d7db93 
This is achieved by triggering a search when the bar opens. This is slightly inefficient but cleanly updates everything that's dependent on the search and reduces code duplication.
@akien-mga akien-mga merged commit ec5bfbd into godotengine:master Oct 30, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@BrianMacIntosh BrianMacIntosh deleted the find-highlight-fix branch October 30, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsjtxietian jsjtxietian jsjtxietian left review comments

@MewPurPur MewPurPur MewPurPur approved these changes

@Paulb23 Paulb23 Paulb23 approved these changes

Assignees
No one assigned
Labels
bug topic:editor
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Search matches are not highlighted when opening search with a selection
7 participants
@BrianMacIntosh @AThousandShips @akien-mga @Paulb23 @jsjtxietian @MewPurPur @Chaosus