Convert script editor dialog to SingletonDialog #1852

rdswift · 2021-06-21T01:04:46Z

Summary

This is a…
- Bug fix
- Feature addition
- Refactoring
- Minor / simple change (like a typo)
- Other
Describe this change in 1-2 sentences: Allow opening Settings dialog while File Naming Script Editor is open.

Problem

JIRA ticket (optional): PICARD-XXX

When the file naming script editor is open from the main screen, the user is unable to open the Settings dialog. This is not intuitive behavior.

Solution

Convert the file naming script editor to a SingletonDialog and switch its parent setting to match the currently active window (main screen or settings dialog).

Action

phw

Finally found enough time and had the head free to look at the code, and actually also test run it on Linux and macOS.

First off: This patch looks good and the important parts work.

On macOS there is an issue with window position:

Open the script editor dialog first
Then open options dialog

Kind of unexpected the script editor now stays behind options.It is in front of options when opening it from options. So there seems to be some issue with the reparenting. Not yet sure what to do about this.

But one thing we actually should change in some way: If you have the script editor open and either then open or close the options dialog you loose all pending changes you had in the dialog, it completely resets.

I understand why this happens when one closes options, because this also closes the dialog and you have code to reopen it. I don't yet fully understand why this happens when options get opened. But in both cases we should ensure somehow that the unsaved state does not get lost.

Also I wonder if the dialog really needs to fully close when closing options. Maybe we can unparent it before the options actually close?

picard/ui/options/renaming.py

picard/ui/scripteditor.py

rdswift · 2021-06-24T16:30:46Z

Kind of unexpected the script editor now stays behind options.It is in front of options when opening it from options. So there seems to be some issue with the reparenting. Not yet sure what to do about this.

Perhaps we need to close it and re-open it from Options. It's disappointing that the re-parenting doesn't work.

But one thing we actually should change in some way: If you have the script editor open and either then open or close the options dialog you loose all pending changes you had in the dialog, it completely resets.

I understand why this happens when one closes options, because this also closes the dialog and you have code to reopen it. I don't yet fully understand why this happens when options get opened. But in both cases we should ensure somehow that the unsaved state does not get lost.

I think it's forcing a load() after re-parenting when opening Options. I'll have a look at how to avoid that. For sure the changes will be lost if it closes and re-opens (as is currently the case when closing Options), so that may require more thought.

Also I wonder if the dialog really needs to fully close when closing options. Maybe we can unparent it before the options actually close?

I tried that and closing Options still destroys the script editor dialog object (without closing it so that the class still thinks there is an active dialog). It appears that when the parent is set to Options, the script editor dialog gets added to the list of objects to clean up, but changing the parent doesn't remove it from the list. That's why I ended up closing it when Options is closed. I agree that it would be best if we could keep it open, but I haven't found a way to do that.

rdswift · 2021-06-24T17:37:50Z

I made a couple of changes to avoid doing a full reload when changing parents, and everything seems to be working here on my Windows 7 system. It now retains changes in progress when opening and closing Options. Unfortunately, I don't think this will resolve the window placement issue on the mac.

phw

I tried to get this working on macOS, without success so far. What I notice is that the parent change on the dialog happens before the options dialog is actually visible, and I think that's the culprit.

So to fix this I tried moving the handling to OptionsDialog by implementing openEvent and also overwriting open. But both did not give any better result. The only thing I achieved was that then the script editor also showed up behind options on Windows.

Strangely when I did remove the parent change when opening options the dialog properly came to front when triggered by the button in renaming options.

Currently I am out of ideas what to try :(

picard/ui/mainwindow.py

rdswift · 2021-06-25T16:21:53Z

Currently I am out of ideas what to try :(

You may have already tried this, but I just pushed a commit that moves the call to open the script editor from the main window to Options (which seems to work well here on my Win 7 system). It may be a case of the origin of the call affecting things.

zas · 2021-06-29T08:03:23Z

I found a reproducible exception while testing:

open script editor from main menu
open Options dialog
press Edit Script in Options (it will make Script Editor dialog to get focus)
close Script Editor

Traceback (most recent call last):
  File "./picard/ui/options/renaming.py", line 203, in script_editor_dialog_close
    if not self.script_editor_dialog.loading:
AttributeError: 'NoneType' object has no attribute 'loading'

picard/ui/mainwindow.py

phw · 2021-07-01T18:07:20Z

Just a quick update that this does still not work on macOS. I haven't found the time yet to experiment here in depth.

rdswift · 2021-07-01T22:37:31Z

Just a quick update that this does still not work on macOS. I haven't found the time yet to experiment here in depth.

When you're testing, you might try changing Line 507 of picard/ui/scripteditor.py from flags = self.windowFlags() | QtCore.Qt.Window to flags = self.windowFlags() | QtCore.Qt.Window | QtCore.Qt.Tool or possibly just flags = self.windowFlags() | QtCore.Qt.Tool. Accrding to the description at https://doc.qt.io/qt-5/qt.html#WindowType-enum

... On macOS, tool windows correspond to the NSPanel class of windows. This means that the window lives on a level above normal windows making it impossible to put a normal window on top of it. By default, tool windows will disappear when the application is inactive. This can be controlled by the Qt::WA_MacAlwaysShowToolWindow attribute.

I'm kind of grasping at straws here, but it sounds like it might be promising. Sorry I can't test it here, but I don't have access to a macos system.

rdswift · 2021-07-03T19:32:16Z

I tracked the events triggered when opening the OptionsDialog object to try to identify the latest event to use for triggering the re-parenting of the ScriptEditorDialog. It appears that the QEvent.Enter event may be the one to use to delay the re-parenting as long as possible to allow the window to become active on macos.

The list of triggered events on my Windows 7 system when opening and then closing OptionsDialog is:

000001: QEvent.Move (13)
000002: QEvent.PlatformSurface (217)
000003: QEvent.WinIdChange (203)
000004: QEvent.WindowIconChange (34)
000005: QEvent.Polish (75)
000006: QEvent.DynamicPropertyChange (170)
000007: QEvent.ChildPolished (69)
000008: QEvent.ChildPolished (69)
000009: QEvent.Move (13)
000010: QEvent.Resize (14)
000011: QEvent.Show (17)
000012: QEvent.Move (13)
000013: QEvent.Resize (14)
000014: QEvent.CursorChange (183)
000015: QEvent.ShowToParent (26)
000016: QEvent.ZOrderChange (126)
000017: QEvent.WindowActivate (24)
000018: QEvent.ActivationChange (99)
000019: QEvent.Paint (12)
000020: QEvent.PolishRequest (74)
000021: QEvent.LayoutRequest (76)
000022: QEvent.UpdateLater (78)
000023: QEvent.UpdateRequest (77)
000024: QEvent.Paint (12)
000025: QEvent.Enter (10)
000026: QEvent.LayoutRequest (76)
000027: QEvent.LayoutRequest (76)
000028: QEvent.LayoutRequest (76)
000029: QEvent.LayoutRequest (76)
000030: QEvent.CursorChange (183)
000031: QEvent.UpdateRequest (77)
000032: QEvent.Paint (12)
000033: QEvent.CursorChange (183)
000034: QEvent.UpdateRequest (77)
000035: QEvent.Paint (12)
000036: QEvent.UpdateRequest (77)
000037: QEvent.Paint (12)
000038: QEvent.UpdateRequest (77)
000039: QEvent.Paint (12)
000040: QEvent.UpdateRequest (77)
000041: QEvent.UpdateRequest (77)
000042: QEvent.Paint (12)
000043: QEvent.WindowDeactivate (25)
000044: QEvent.ActivationChange (99)
000045: QEvent.Hide (18)
000046: QEvent.HideToParent (27)
000047: QEvent.DeferredDelete (52)

Most of the events between 000026 and 000042 inclusive were a result of moving the mouse pointer to the Cancel button on the dialog.

EDIT: I used a simple class that I threw together to look after logging the events to a file, in case you want to try logging the events generated on the macos to confirm the order. The class can be found at:
https://gist.github.com/rdswift/c6943bb6767878a86506915e68807745

phw · 2021-07-05T06:25:36Z

@rdswift I tried again to get the workflow of having the script editor dialog open first, then opening options on macOS running. No matter what I tried, the script editor stays behind options. One can interact with it (at least as far as it is visible), but cannot get it to front.

I also tried making the window modal before re-parenting. Then it was in front, but exclusive and one could not interact with options. Changing modal back after showing the window does not change this. Changing it back before showing has no effect, resulting in the original behavior.

Not really sure what to do about this.

rdswift · 2021-07-05T13:06:01Z

That's disappointing. Perhaps as a really ugly hack we could close the script editor then reopen it again, but that would likely not allow us to save any unsaved changes first. I'll do some experimenting to see if I can come up with anything better.

rdswift · 2021-07-05T15:49:53Z

I wonder if the answer might lay in https://doc.qt.io/qt-5/macos-issues.html#using-native-cocoa-panels ?

phw · 2021-07-22T06:41:34Z

@rdswift I had no luck with getting this to work on macOS. I was hopeful about your latest patches, but they did not work. Also it is not solely a timing issue as I had first thought, delaying the opening does not work. I found not way to get the script editor show up in front of the option dialog if the script editor is opened earlier.

I tend toward just accepting this as a known issue on macOS. @zas What do you think?

zas · 2021-07-22T07:08:37Z

@zas What do you think?

Well, if this patch improves user experience on all platforms but on macos, I'd say it is still an improvement, and should be merged.
We can add a ticket about the macos-specific issue.

phw · 2021-07-22T07:38:55Z

Ok, in this case let's merge, but we should skip the last three commits. I can test this one more time and then merge this locally, and also add the ticket.

rdswift · 2021-07-22T14:52:15Z

@rdswift I had no luck with getting this to work on macOS. I was hopeful about your latest patches, but they did not work.

Darn. I had high hopes for that solution. I'm all out of suggestions.

Ok, in this case let's merge, but we should skip the last three commits.

I'll roll back to commit 265704a and rebase to get rid of the conflicts then.

phw

LGTM

I added PICARD-2249 about the macOS dialog issue.

zas

Lgtm

rdswift force-pushed the make_script_editor_singletondialog branch from 23bb31b to e00ac02 Compare June 21, 2021 01:10

rdswift requested review from phw and zas June 21, 2021 14:52

phw requested changes Jun 24, 2021

View reviewed changes

picard/ui/options/renaming.py Outdated Show resolved Hide resolved

picard/ui/scripteditor.py Show resolved Hide resolved

picard/ui/scripteditor.py Outdated Show resolved Hide resolved

phw reviewed Jun 25, 2021

View reviewed changes

picard/ui/mainwindow.py Outdated Show resolved Hide resolved

rdswift marked this pull request as ready for review June 25, 2021 16:26

rdswift removed the request for review from zas June 28, 2021 20:43

zas requested changes Jun 29, 2021

View reviewed changes

picard/ui/mainwindow.py Outdated Show resolved Hide resolved

picard/ui/mainwindow.py Show resolved Hide resolved

rdswift added 8 commits July 22, 2021 09:00

Convert script editor dialog to SingletonDialog

6bdaa88

Remove unused code.

3a9078a

Move automatic parent change to SingletonDialog.

07da522

Avoid load() when using existing instance. Remove redundent code.

2d16bee

Add get_page() helper to OptionsDialog

48c79ce

Move show_script_editing_page() call from Main Window to Options

459d30f

Remove unneeded check causing exception

d5626f4

Remove redundant self.script_editor_is_open variable

2a464ae

rdswift added 4 commits July 22, 2021 09:10

Use list to track which script editor dialog signals to disconnect

d4c199a

Move ScriptEditorDialog re-parenting to follow OptionsDialog activation

c8f9f05

Move ScriptEditorDialog re-parenting to first OptionsDialog paint event

26f3759

Move ScriptEditorDialog re-parenting to first OptionsDialog enter event

0277ac7

rdswift force-pushed the make_script_editor_singletondialog branch from 367786d to 0277ac7 Compare July 22, 2021 15:25

phw previously approved these changes Jul 27, 2021

View reviewed changes

phw requested a review from zas July 27, 2021 16:00

Merge branch 'master' into make_script_editor_singletondialog

9b69f20

rdswift dismissed phw’s stale review via 9b69f20 July 27, 2021 16:42

zas approved these changes Jul 27, 2021

View reviewed changes

phw merged commit 51db740 into metabrainz:master Jul 27, 2021

rdswift deleted the make_script_editor_singletondialog branch July 27, 2021 19:37

rdswift mentioned this pull request Jul 29, 2021

Catch exception when re-parenting SingletonDialog instance #1873

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert script editor dialog to SingletonDialog #1852

Convert script editor dialog to SingletonDialog #1852

rdswift commented Jun 21, 2021

phw left a comment

rdswift commented Jun 24, 2021

rdswift commented Jun 24, 2021

phw left a comment

rdswift commented Jun 25, 2021 •

edited

zas commented Jun 29, 2021

phw commented Jul 1, 2021

rdswift commented Jul 1, 2021

rdswift commented Jul 3, 2021 •

edited

phw commented Jul 5, 2021

rdswift commented Jul 5, 2021

rdswift commented Jul 5, 2021

phw commented Jul 22, 2021

zas commented Jul 22, 2021

phw commented Jul 22, 2021 •

edited

rdswift commented Jul 22, 2021

phw left a comment

zas left a comment

Convert script editor dialog to SingletonDialog #1852

Convert script editor dialog to SingletonDialog #1852

Conversation

rdswift commented Jun 21, 2021

Summary

Problem

Solution

Action

phw left a comment

Choose a reason for hiding this comment

rdswift commented Jun 24, 2021

rdswift commented Jun 24, 2021

phw left a comment

Choose a reason for hiding this comment

rdswift commented Jun 25, 2021 • edited

zas commented Jun 29, 2021

phw commented Jul 1, 2021

rdswift commented Jul 1, 2021

rdswift commented Jul 3, 2021 • edited

phw commented Jul 5, 2021

rdswift commented Jul 5, 2021

rdswift commented Jul 5, 2021

phw commented Jul 22, 2021

zas commented Jul 22, 2021

phw commented Jul 22, 2021 • edited

rdswift commented Jul 22, 2021

phw left a comment

Choose a reason for hiding this comment

zas left a comment

Choose a reason for hiding this comment

rdswift commented Jun 25, 2021 •

edited

rdswift commented Jul 3, 2021 •

edited

phw commented Jul 22, 2021 •

edited