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

Avoid duplicating the "Filters" section #79650

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Avoid duplicating the "Filters" section #79650

merged 1 commit into from
Aug 2, 2023

Conversation

garychia
Copy link
Contributor

@garychia garychia commented Jul 19, 2023 •
edited by KoBeWi
Loading

Fixes #79630
Bugsquad edit: Fixes #79663 Supersedes #79664

@garychia garychia requested a review from a team as a code owner July 19, 2023 08:32
@kleonc kleonc added this to the 4.2 milestone Jul 19, 2023
@kleonc kleonc added cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 19, 2023
@jonhermansen jonhermansen mentioned this pull request Jul 19, 2023
Closed
@jonhermansen
Copy link

I found a related bug while testing this change, but actually it has existed for some time.

I opened a new PR that includes both fixes but if that's not right, let me know. I would be happy to raise a separate bug and create a PR including just the icon fix.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 21, 2023

I think the original code makes little sense. Why is this method called on every popup if the menu is supposed to always be the same? It should only be initialized once, right after filter is created.

@garychia
Copy link
Contributor Author

I agree. This seems to be a better solution.

@garychia
Copy link
Contributor Author

I tried to initialize the menu right after filter is created, but the images on the menu were broken:
image
The original code seems to have taken this into consideration.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 22, 2023

The icons should be assigned in NOTIFICATION_THEME_CHANGED.

KoBeWi
KoBeWi reviewed Jul 22, 2023
View reviewed changes
editor/scene_tree_dock.cpp Outdated
@@ -1386,6 +1386,11 @@ void SceneTreeDock::_notification(int p_what) {

filter->set_right_icon(get_theme_icon(SNAME("Search"), SNAME("EditorIcons")));

PopupMenu *p_menu = filter->get_menu();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PopupMenu *p_menu = filter->get_menu();
PopupMenu *filter_menu = filter->get_menu();

p_ prefix should be used only for parameters.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 22, 2023 •
edited
Loading

You should remove get_theme_icon() calls from _append_filter_options_to(). NOTIFICATION_THEME_CHANGED is automatically received when node is added to tree, so it handles initialization too.

KoBeWi
KoBeWi reviewed Jul 23, 2023
View reviewed changes
editor/scene_tree_dock.cpp
@@ -1386,6 +1386,11 @@ void SceneTreeDock::_notification(int p_what) {

filter->set_right_icon(get_theme_icon(SNAME("Search"), SNAME("EditorIcons")));

PopupMenu *filter_menu = filter->get_menu();
filter_menu->set_item_icon(filter_menu->get_item_idx_from_text(TTR("Filters")), get_theme_icon(SNAME("Search"), SNAME("EditorIcons")));
Copy link
Member

@KoBeWi KoBeWi Jul 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if there is a better way to get this item than using the text.

@YuriSizov YuriSizov changed the title Avoid duplicating the 'Filters' section Avoid duplicating the "Filters" section Jul 27, 2023
@YuriSizov YuriSizov self-requested a review July 27, 2023 12:35
@akien-mga akien-mga mentioned this pull request Aug 2, 2023
Closed
@akien-mga akien-mga merged commit 1cc377b into godotengine:master Aug 2, 2023
@garychia garychia deleted the filters_section branch August 2, 2023 12:55
@YuriSizov
Copy link
Contributor

Looks like we forgot to say "thanks", so here's Thanks!
Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi approved these changes

@YuriSizov YuriSizov Awaiting requested review from YuriSizov

Assignees
No one assigned
Labels
bug topic:editor
Projects
None yet
Milestone
4.2
6 participants
@garychia @jonhermansen @KoBeWi @YuriSizov @akien-mga @kleonc