Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Work Files: Fix parenting of save prompt QMessageBox #2784

Merged
merged 2 commits into from
Feb 22, 2022

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Feb 22, 2022

Brief description

This makes the save prompt messagebox an actual child to the parent Qt UI element.

Resolves issue

This fixes the save prompt appearing behind the workfiles UI in Fusion.

Description

Original "crash" case

Setting the windowFlags without the original messagebox.windowFlags() was the culprit as to why the messagebox previously wouldn't show when parented. Likely because then it's missing the Dialog window flag and thus would try to embed itself into the parent UI, which you then cannot exec()

Additional Context

(cherry picked from commit 290e2b6)

Testing notes:

  1. Test whether save prompt (when current scene has modifications and you are opening a file through Work Files) works as expected
  2. Please test Blender integration, since original code mentioned "crashes" in Blender. ⚠️

I tested in Fusion 17 and Maya 2020 - those worked fine. ✅

- Setting the windowFlags without the original messagebox.windowFlags() was the culprit as to why the messagebox previously wouldn't show when parented. Likely because then it's missing the Dialog window flag and thus would try to embed itself into the parent UI, which you then cannot exec()

(cherry picked from commit 290e2b6)
@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 22, 2022

@antirotor

afbeelding

It's a trend. :) Although not as nice as this

Copy link
Member

@iLLiCiTiT iLLiCiTiT left a comment

Choose a reason for hiding this comment

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

The crash in blender is "occasional" so it is hard to test. I would recommend to keep store it in the attribute, as it won't affect your change.

EDITED:
I don't think it is still an issue but blender is already so broken so why to break it even more? :)

Copy link
Member

@iLLiCiTiT iLLiCiTiT left a comment

Choose a reason for hiding this comment

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

I accidentally approved.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 22, 2022

@iLLiCiTiT since the original comment mentioned this:

(setting parent doesn't work as it hides the message box)

I felt like the issue originally in blender was purely due to the parent relationship not existing in the first place and thus the messagebox getting garbage collected. (Due to how Blender would process events even if you'd explicitly call exec() I think).

Should we maybe just try and see if this now holds up in Blender as is? Maybe with just doing some preliminary testing - because I do recall the original crash was somewhat reproducable.

The original implementation of storing the messagebox came from here which also describes that. It also came up here as:

A reference to pop-up windows (like the 'Save changes?' in Work Files) is needed, else they are garbage collected and immediately disappear. I now use self.message for example instead of just message. So this one is fixed in my branch.

Which should now also be solved with the parenting.

Other issues related to crashes in blender all seemed to be related to the Window instead of the Message Box.


@simonebarbieri and @Tilix4 any chance you could try and get this to crash in Blender? :) Specifically using the Work Files such that it'd prompt to save the current scene. E.g. currently having an active scene open with unsaved modifications, and then clicking in the workfiles tool to open an existing file?

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 22, 2022

I can confirm that it works on windows (even without messagebox.windowFlags()) but it would be good to try it on linux where both Blender and Qt are using totally different code for crutial parts.

For me it is worth it to store the dialog into attribute just to make sure it is not garbage collected. It affects nothing.

- This is NOT done because the original crash was reproducible - but just out of pure legacy reasons for if the error might still occur. It would be worth looking into whether the crash can still be reproduced in recent Blender versions without this logic.
@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 22, 2022

For me it is worth it to store the dialog into attribute just to make sure it is not garbage collected. It affects nothing.

I'd rather avoid keeping around legacy code that we might not need. Anyway, reverted that with this commit bb10520

I did include this in the commit message:

This is NOT done because the original crash was reproducible - but just out of pure legacy reasons for if the error might still occur. It would be worth looking into whether the crash can still be reproduced in recent Blender versions without this logic.

Just so it won't confuse anyone in the future why that got added back in. Let me know if you still want to change something. :)

@iLLiCiTiT iLLiCiTiT merged commit 9386c47 into ynput:develop Feb 22, 2022
@BigRoy BigRoy mentioned this pull request Feb 22, 2022
8 tasks
@BigRoy BigRoy deleted the workfiles_fix_save_prompt_parent branch March 20, 2024 15:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants