Skip to content

Remove extra NOTIFICATION_VISIBILITY_CHANGED notifications in docks#113070

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
lodetrick:dock-too-many-notifications
Nov 25, 2025
Merged

Remove extra NOTIFICATION_VISIBILITY_CHANGED notifications in docks#113070
Repiteo merged 1 commit into
godotengine:masterfrom
lodetrick:dock-too-many-notifications

Conversation

@lodetrick
Copy link
Copy Markdown
Contributor

See #113034 (comment)
Related: #113024

When upgrading docks to the new system, I first noticed that docks treated the NOTIFICATION_{ENTER, EXIT}_TREE like constructors and destructors, only being called once. When they used the EditorBottomPanel, this was valid, but now that they use the EditorDockManager, this is no longer true. Thus, any actions that must be done once, should be moved.

In #113034 (comment), the underlying bug was a case where NOTIFICATION_VISIBILITY_CHANGED was used to know when the panel was enabled or disabled. This is valid if the notification only happened when both the enabled and visible, and disabled and not visible. However, due to an ordering issue with EditorDockManager, the NOTIFICATION_VISIBILITY_CHANGED(true) was emitted when closing the dock, and NOTIFICATION_VISIBILITY_CHANGED(false) was emitted when opening the dock, leading to incorrect assumptions.

This PR swaps the order of operations so that the notification is only emitted correctly, to preserve assumptions. This has the benefit of removing unnecessary operations when opening and closing docks.

EditorDock::open() EditorDock::close()
Before: NOTIFICATION_VISIBILITY_CHANGED
NOTIFICATION_EXIT_TREE
NOTIFICATION_VISIBILITY_CHANGED
NOTIFICATION_ENTER_TREE
NOTIFICATION_VISIBILITY_CHANGED
NOTIFICATION_VISIBILITY_CHANGED
NOTIFICATION_EXIT_TREE
NOTIFICATION_VISIBILITY_CHANGED
NOTIFICATION_ENTER_TREE
NOTIFICATION_VISIBILITY_CHANGED
After: NOTIFICATION_EXIT_TREE
NOTIFICATION_ENTER_TREE
NOTIFICATION_VISIBILITY_CHANGED
NOTIFICATION_VISIBILITY_CHANGED
NOTIFICATION_EXIT_TREE
NOTIFICATION_ENTER_TREE

@lodetrick lodetrick requested a review from a team November 23, 2025 01:22
@lodetrick lodetrick changed the title Fix extra NOTIFICATION_VISIBILITY_CHANGED notifications in docks Remove extra NOTIFICATION_VISIBILITY_CHANGED notifications in docks Nov 23, 2025
@Repiteo Repiteo added this to the 4.x milestone Nov 23, 2025
@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Nov 23, 2025

Something bad is happening when you close Scene dock in right UR slot:

godot.windows.editor.dev.x86_64_ygLeHgtF2d.mp4

It's somewhat reproducible without this PR, but it's not as extreme.

@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Nov 23, 2025

I realized a major flaw in EditorDockManager. closed_dock_parent is not invisible. If it was, this problem wouldn't exist and the order of making things invisible wouldn't matter, because nodes reparented to closed dock parent couldn't become visible. We should still hide them though, so is_visible() is false.

@lodetrick lodetrick force-pushed the dock-too-many-notifications branch from 150c8ea to 62eeecd Compare November 24, 2025 11:45
@lodetrick
Copy link
Copy Markdown
Contributor Author

lodetrick commented Nov 24, 2025

I made it so that the closed_dock_parent is a new hidden control under the gui_base.

When testing I came across this error when I clicked Editor -> Editor Layout -> Default but I was able to reproduce it in master so I don't think its related.

ERROR: The active_submenu_index should never be -1 when _submenu_hidden is entered.
   at: _submenu_hidden (scene/gui/popup_menu.cpp:2242)

Edit: Nevermind it's happening when you stop hovering over a menu item so very unrelated

@KoBeWi KoBeWi modified the milestones: 4.x, 4.6 Nov 24, 2025
@Repiteo Repiteo merged commit 912da56 into godotengine:master Nov 25, 2025
20 checks passed
@Repiteo
Copy link
Copy Markdown
Contributor

Repiteo commented Nov 25, 2025

Thanks!

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.

3 participants