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

Fix broken bottom panel switching #72420

Merged

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jan 30, 2023

The cause of the crash is simple recursion. Switching events bound to buttons on the bottom panel were being called recursively by inspector updates.

The inability to transition to the animation tab is due to the inspector update, which can be resolved by the restriction that bottom tab switching is allowed only once in the same frame. This means that the pending update method of #68498 cannot be used.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 30, 2023

This does fix the issues, but while testing I discovered a new buggy behavior (which is either exposed or caused by this PR).

  1. First make sure that Animation is active (i.e. the keys are visible in the inspector)
  2. Add a theme to some Control node and unfold it
  3. You will see Theme editor
  4. Switch to Animation tab
  5. The inspector should be displaying: Control node properties, keys, unfolded theme
  6. Fold Theme resource in the inspector
  7. Switch to Theme editor
  8. The Theme tab disappears, but the editor opens normally
  9. Ghost editor 👻
    image

Although I guess it's still better than being unable to switch to Animation or crashing.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Approving, but this might be a temporary solution, or at least it requires further fixes.

@TokageItLab
Copy link
Member Author

Ghost editors seem to have existed before this PR...

@TokageItLab TokageItLab force-pushed the fix-animation-editor-plugin branch from 6adf857 to 8668c06 Compare January 31, 2023 09:47
@TokageItLab TokageItLab force-pushed the fix-animation-editor-plugin branch from 8668c06 to fc22583 Compare January 31, 2023 10:34
@TokageItLab
Copy link
Member Author

TokageItLab commented Jan 31, 2023

This fixes the switching bug, but I found a side effect in the EditorPlugin which may spam switching in the bottom panel, may cause problems with the bottom panel closing unwillingly.

The ones I have encountered so far are SpriteFramesEditor and ThemeEditor, but they may need to be addressed individually. Also, the ghost editor has existed prior to this PR, but that may need to be fixed as well.

@akien-mga
Copy link
Member

Thanks!

@TokageItLab
Copy link
Member Author

Note: The issue with the theme editor seemed to be a different problem #72271

@JulianHeuser
Copy link
Contributor

This seems to have caused another issue - I'm using a custom editor plugin that updates whenever a node is selected. However, after beta 17, the _edit function is only called if the dock updates. Previously, it would be called every time a new node was selected.

This means I have to select a node that causes my custom dock to hide, then select another node that makes it appear again to get it to update at all.

I don't see exactly how this could have been caused by this fix, but I don't see any other PRs that changed EditorPlugin since beta 16 (where everything worked as expected)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants