-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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
Extract editor scene tabs into their own component #80490
Extract editor scene tabs into their own component #80490
Conversation
} | ||
|
||
void EditorSceneTabs::_scene_tab_changed(int p_tab) { | ||
tab_preview_panel->hide(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to work for some reason. (although it wasn't before too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular steps I could try?
If I hover over a tab with a preview, when move over to another tab with no preview (so that the original preview remains), the moment I click on the other tab, the preview disappears. Not sure if it goes this path, but I assume so? So it seems to work correctly to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's either random or only works for the first tab:
godot.windows.editor.x86_64_Fk8x1MoNP8.mp4
Then again, if it's a copy-pasted code that was broken before then it can be fixed later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, absolutely. I was just unable to reproduce it myself, so for future reference this is useful. Thanks!
EditorNode::disambiguate_filenames(full_path_names, disambiguated_scene_names); | ||
|
||
// Workaround to ignore the tab_changed signal from the first added tab. | ||
scene_tabs->disconnect("tab_changed", callable_mp(this, &EditorSceneTabs::_scene_tab_changed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object.set_block_signals()
could be used instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we resolve this conversation? I assume that you don't want to use @KoBeWi's advice for this case, @YuriSizov ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should resolve this because that's still something to look into. But, as I've mentioned below, this is not in the scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be working correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a rebase
Just FYI, this is a mostly one-to-one extraction, so I didn't do a quality pass on the code. I will apply small style suggestions, but reworking problems with this class is outside of the scope of this PR (which I would define as decoupling of EditorNode parts). |
7b50ddc
to
6ab3400
Compare
Rebased (fixing a typo from #79382 (comment)) and applied some of the suggested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good!
Thanks! |
Another one of my extraction PRs for a part of
EditorNode
. It's not a huge part, but it's still worthwhile to properly abstract it.There should be no behavioral changes after this PR. As an extra, I noticed a couple of methods were bound for no reason and removed their binds. One was used in a deferred call, so I reworked it. Others don't seem to be used in deferred calls or UndoRedo, so this should be safe.