Fix crash when using "Close All Tabs" #79917
Conversation
|
I have tried to not break the legacy code, and hopefully I haven't. I have tested this change manually, but it would be really good if we could have tests for the GUI as well. I can see that the tests are using mocked display server and |
|
@YuriSizov, can we port this to 4.1 branch? I know this is not something of a blocker, but I would really like to have it on 4.1.x since I am using this version for my purposes. On the other hand, there is really nothing that keeps me on 4.1. I can always downgrade to 4.0. But if a backport comes to mind, just note that, while the first issue reproducible on 4.1.x, the second one is only reproducible on master branch. I will add this info as well in the description. What I am trying to say is that, if someone decides to backport this change, we need to separate this into two PRs. |
|
@hvarga Generally speaking, you shouldn't mix together different fixes to begin with. In this case they are closely related, so I haven't asked you to split them. But since you raise that concern yourself, it's worth a mention. It would also be great if you open issue reports first, so PRs can be properly linked to them, especially since these are different issues affecting different versions and thus different users. It doesn't mean you need to wait for something, you can just open the report and the relevant PR at the same time. We normally cherry-pick PRs that are trivially cherry-pickable and only ask for backports if this is not the case. So that's another reason to make separate PRs for separate fixes. |
hvarga
force-pushed
the
fix-scene-tab-close
branch
from
80ef847
to
7693b7a
Compare
|
Understood. I will separate this into two PRs and also open issues for them. |
hvarga
force-pushed
the
fix-scene-tab-close
branch
from
7693b7a
to
51923fc
Compare
|
@YuriSizov I am done with separating this change into two PRs. The second fix is done in #79945. Sorry for the noise. |
YuriSizov
added
the
cherrypick:4.1
No problem at all! You did a lot of work identifying and describing problems despite this being your first engine contribution. Hiccups can happen, and it all looks perfect now :) |
|
Thanks! And congrats on your first merged Godot PR! |
|
Cherry-picked for 4.1.2. |
YuriSizov
removed
the
cherrypick:4.1
Fixes #79943.
I have fixed this in a way so that this deferred call to
_set_main_scene_stateis done only once at the very end after all tabs are closed.There is a potential for further improvement which can speed-up the closing of the tabs. I could be wrong, and probably I am due to my lack of knowledge about Godot, but I don't see any need to update the editor for each of the closed tab. The update can be done only once at the end when closing of scene tabs is complete. But I will leave this decision to guys smarter then me like Tomasz Chabora who made this original refactor.