Fix inconsistent usage of apply transform dialog #1712
Conversation
While the main problem I sought to fix was that while using the move tool, you couldn't drag a keyframe on the timeline, the underlying issue was caused by the layerSwitching logic which wasn't fully thought through. The new logic depends on the tool or widget to tell the editor that a user action is required before the execution can continue, this should be done using the new "requireUserAction" and "RequestUserAction" function. The "RequestUserAction" method will check if the flag has been enabled and in that case, call the respective tool that requires it. It's then up to the tool to create a dialog for the said action. To make the dialog appear consistently when interacting with widgets that would apply the transformation, we apply an invisible widget on top of the base dock widget which will appear when the UI should be blocked. This makes it possible for the respective QWidget to get the QEvent instead of its children. Fixes: - Fix move tool blocking timeline keyframe dragging action - Fix many other cases where the transform dialog should have appeared but didn't. - Fixed that when the action popup disappears, the the timeline press event would be called but not the release event, leaving the widget in a pressed state where the scrubber would be moved upon hover.
Apparently it's not guaranteed that a release event is fired, so to avoid weird states we reset state when the mouse leaves the widget.
3ca1f8d to
ab81550
Compare
There was a problem hiding this comment.
I just took a look at the changes, but there are still some rough edges.
Firstly, there’s a bug when you are using the select tool and switching to the move tool temporarily using the Alt key and the disabled widgets do not get re-enabled after moving the selection. But even while the widgets were disabled, it’s still possible to use keyboard shortcuts to switch tools and probably also for other actions – maybe relying purely on the UI to prevent those actions is a bit too brittle?
Secondly, I found it a bit confusing to disable all those widgets. To me, disabled normally means that I can’t interact with those widgets, but in this case I can – causing the confirmation dialog to show up (which I noticed isn’t modal btw, not on master either). I reckon keeping the widgets interactive/enabled and simply showing the dialog on interaction would be sufficient.
Lastly, I think requireUserAction and requestUserAction could use some clarification. It’s easy to forget which one is which and not documented what kinds of interaction they are meant to prevent.
Other than that, the code does look good though. It’s definitely good to have this case handled more comprehensively than it is now.
Edit: I should probably also mention that I can still interact with the main menu even though it should be disabled. But that seems to be an issue with my Global Menu widget, not with your code.
|
Thanks for reviewing Jakob! I've removed the disable logic now, I previously had it for some event handling but it's redundant now, also I agree that it might not make much sense to disable the widget when you can't interact with them anyway because of the overlay. Ready for second round 😄 |
J5lx
left a comment
There was a problem hiding this comment.
Code looking good all in all, but there’s still the issue that you can continue to use keyboard shortcuts even when user action is required, which I mentioned before. For example, you can use shortcuts to navigate between frames while there is a pending transformation. Would it maybe be more reliable to trigger the user action right from the layer/tool switching code rather than just trying to disable the various different ways a layer/tool change can be effected by the user?
|
Hmm I should have asked before.. how do you trigger the shortcuts to navigate between frames? as far as i've tried all shortcuts are disabled except the ones we explicitly allow, eg. selection? Could you mention which shortcuts you mean specifically and how to trigger them, I don't seem to be able to use anything but the shortcuts that are available to the move tool. I tried to tie it to layer or tool switching previously but the problem I ran into is that if you want to be able to give the user control to either accept, deny or cancel, then you need to be able to handle a case where the user cancelled the action which would require various checks in the respective places where you're trying to block the execution. It was rather messy, now it's tied to the tool and the user interaction but it does require making sure either the shortcut or UI is properly disabled. |
|
I’m simply pressing the . and , keys which I believe are the default shortcuts for that. I’m not really doing anything special, just pressing those keys like normal.
Hmm I see… 🤔 |
Interesting 🤔 maybe it's a platform specific issue, I would have expected that given that the menu items has been disabled, using those shortcuts would result in nothing, at least that's what I see on Mac OS. I guess I'll try to boot up my windows machine in one of the following days and try to reproduce and read up on qmenu and its disable logic. |
|
I also just tried disabling my global menu to see if the shortcut thing is related to the bug with the disabled toplevel menus that I mentioned in my initial review (which doesn’t seem to be the case) and I realised that it might technically also make sense to keep some entries enabled, e.g. File→Quit or Edit→Clear frame. Though of course that would also be additional effort… maybe it would make sense to build something more general to let the Editor reliably communicate available actions to the UI… though that might just be overkill for this PR… |
|
The issues should have been addressed now. Triggering a shortcut will now trigger the dialog. I'm also now only disabling the import menu as opposed to the entire file menu. I'm still not sure about the let Editor tell UI what is possible, it technically makes sense but the amount of work required to pull it off is not trivial as far as I can see and we only have the move tool using it currently, making me think that we could put the effort to better use. I also thought again about giving the responsibility to the editor but I always run into the same problem, if you need to be able to cancel an action then you either need to intercept all calls or have checks in various parts of the core which a return value that also has to be acted upon. It's a lot more trivial to stop it on the UI side, because it's the point of entry. The PR focuses on fixing the inconsistent behavior of the dialog (which causes bugs), which I think it achieves, when that's said, there might be better ways or more resilient methods that surely can be implemented when the need arises. The clear frame is still disabled but that's a deliberate choice given that you've started a transformation, this I think it makes no sense wanting to clear your entire keyframe. |
J5lx
left a comment
There was a problem hiding this comment.
After trying it out, I like the solution with the shortcut filter more than I would have expected. Nice work! I like the menu change too. That said, it’s still not quite enough unfortunately since the shortcut filter is only installed on the tool switching actions, whereas other keyboard shortcuts, like the ones for navigating between frames which I mentioned before, can still be used. So I guess one would have to install the filter on all of the other relevant actions as well…? There really is no silver bullet for this issue, huh. sighs
you need to be able to cancel an action then you either need to intercept all calls or have checks in various parts of the core
Yeah that would be the tricky/cumbersome part I suppose… I just wish there was a simple and straightforward solution, lol. Nevertheless, thanks for looking into it.
deliberate choice given that you've started a transformation
Fair point.
|
I'm looking into removing the apply dialog in another PR where i've reworked the select and move tool to handle transformations properly, so i'll close this pr. |
Implement UI block logic for when an action is required
While the main problem I sought to fix was that while using the move tool, you couldn't drag a keyframe on the timeline, the underlying issue was caused by the layerSwitching logic which wasn't fully thought through.
The new logic depends on the tool or widget to tell the editor that a user action is required before the execution can continue, this should be done using the new "requireUserAction" and "RequestUserAction" function. The "RequestUserAction" method will check if the flag has been enabled and in that case, call the respective tool that requires it. It's then up to the tool to create a dialog for the said action.
To make the dialog appear consistently when interacting with widgets that would apply the transformation, we apply an invisible widget on top of the base dock widget which will appear when the UI should be blocked. This makes it possible for the respective QWidget to get the QEvent instead of its children.
Fixes: