Prevent application closing when ok cancel dialog open.

[cmeyer]

No description provided.

[Brow71189]

Is there any benefit from this? For me this sounds like a regression because these windows often hide behind other windows, so a user will attempt to close Swift but it doesn't react for no apparent reason, forcing them to force close it.
Our dialogs are modeless, so why would we want them to become partially modal?

[Tiomat85]

I would echo Andreas' question here. If you have some dialogs that must be answered before continuing (or closing) surely the correct response would be to make that dialog modal rather than modeless. Modeless by design implies no context or answer at the end of it.

Is there a specific instance of an ok/cancel dialog being open that causes a problem?

[cmeyer]

The benefit is fixing nion-software/nionswift#1635

Where do we use ok/cancel dialogs that can prevent closing the software? Maybe we shouldn't be using them in those spots.

[Brow71189]

This PR is not a fix for nion-software/nionswift#1635, if anything it can be considered a workaround.
The problem in nion-software/nionswift#1635 is not that Swift closes even though a dialog is open, the problem is that selecting a missing project is not handled properly.

This PR introduces a regression for all dialogs across the entire software just to implement a poor workaround for an unrelated bug. I don't see how that is something we want to merge.

[Ion-e]

I'm struggling to understand the fix of this ticket, if I have understood this fix correctly, it doesn't fully address nion-software/nionswift#1635, but it seems to more accurately address:
nion-software/nionswift#1441
nion-software/nionswift#1411

I have tested the PR and haven't yet spotted the difference with the previous behaviour. Does swift have any "OK/Cancel" dialog? Or are we referring to "Generate/Cancel"-"Export/Cancel"-"Open/Cancel"....

[cmeyer]

This PR introduces a regression for all dialogs across the entire software just to implement a poor workaround for an unrelated bug. I don't see how that is something we want to merge.

@Brow71189 I'm not sure you've actually looked at this code or tried it out. This PR only affects a single function call show_ok_dialog which is currently only used for displaying dialogs during project switching/opening/closing. You can see this yourself by doing a quick search of show_ok_dialog in the code. By no means is this a regression across all dialogs and you're confusing the discussion with statements like that.

show_ok_dialog is distinct from show_ok_cancel_dialog. show_ok_dialog is used more as a 'something bad has happened with no chance of cancellation and I need you to acknowledge before moving on'.

That said, I can simply move this functionality outside the calls to show_ok_dialog which may be a better solution.

[Ion-e]

To add top my last comment, nion-software/nionswift#1635 seems fixed, not sure if related to this PR specifically so I'll close that ticket. Thanks!

[Tiomat85]

So it stops Swift closing while the dialog is open, which forces you to respond to it, which just seems to be a modal dialog with extra steps? Since that then allows you do interact with most things, but not every thing which would feel more confusing. Your description indicating that you need the user to acknowledge before moving on is the exact use case for modal dialogs so is there a reason we are avoiding that style?

It also would make more sense, as an error dialog, to be a modal show_error_dialog but that is a side-issue.

[cmeyer]

This discussion isn't going anywhere. I'm closing this PR. Please try the workflows yourself and talk to @Ion-e about the purpose of this fix. I will submit a separate PR for the specific case of nion-software/nionswift#1635.

[Ion-e]

@cmeyer Just to clarify. When testing nion-software/nionswift#1635 was fixed in this PR (when open) and not on Master, so I arrived to the conclusion that this PR was fixing the issue of that ticket.
I'm now confused about the resolution of both the ticket and the PR.

[cmeyer]

@Ion-e We can discuss more offline - but I like to keep issues open until the PR is actually merged, just in case the PR is not accepted (as in this case).

FYI - follow-up PR (same solution, just narrowed scope) is here nion-software/nionswift#1685