pencil icon indicating copy to clipboard operation
pencil copied to clipboard

Fix inconsistent usage of apply transform dialog

Open MrStevns opened this issue 2 years ago • 6 comments

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:

  • 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.

MrStevns avatar May 14 '22 15:05 MrStevns

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. As requested i've clarified the usage methods by merging them into requestUserActionIfNeeded which handles both requestUserAction and requireUserAction. I realized that it did not make much sense to first check and then do, when the latter method didn't do anything anyway.

Ready for second round 😄

MrStevns avatar Jun 06 '22 17:06 MrStevns

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. ~In the end it's a mechanism for blocking interaction, thus I think it's okay to tie it to the UI and not the core~

MrStevns avatar Jun 06 '22 20:06 MrStevns

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.

It was rather messy

Hmm I see… :thinking:

J5lx avatar Jun 06 '22 20:06 J5lx

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.

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.

MrStevns avatar Jun 06 '22 20:06 MrStevns

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…

J5lx avatar Jun 06 '22 20:06 J5lx

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.

MrStevns avatar Jun 08 '22 17:06 MrStevns

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.

MrStevns avatar Aug 17 '22 05:08 MrStevns