Skip to content

Fix OptionalMenu highlighting in loader#518

Closed
iLLiCiTiT wants to merge 6 commits into
getavalon:masterfrom
ynput:highlight_update_fix
Closed

Fix OptionalMenu highlighting in loader#518
iLLiCiTiT wants to merge 6 commits into
getavalon:masterfrom
ynput:highlight_update_fix

Conversation

@iLLiCiTiT
Copy link
Copy Markdown
Contributor

Issue to solve:

  • OptionMenu sometimes doesn't get mouseMoveEvent and is ignoring mouseEnterEvent, hence it the highlight of actions isn't triggered sometimes.
  • mouse release triggers action where cursor currently is rather than the on where it originally clicked

Suggestion:

  • re-implemented mouse movement (enter, leave, move) and press (pressed, released) event handling in OptionalActionWidget instead of OptionalMenu
    • OptionalMenu currently must iterate through all it's actions to filter them but Qt app does that itself, so widgets deal with only their events
  • it is now possible to trigger action only if release event happened on the same action where the mouse was pressed. But hovering to option box after mouse is pressed still works.
  • OptionalMenu was removed since all it's re-implemented methods were moved
  • OptionBox was changed from QWidget to QLabel since it's only child was QLabel

@iLLiCiTiT iLLiCiTiT requested a review from davidlatwe January 24, 2020 07:39
@davidlatwe
Copy link
Copy Markdown
Collaborator

Sorry for late reply, was on vacation (Lunar New Year 🎈).

  • OptionMenu sometimes doesn't get mouseMoveEvent and is ignoring mouseEnterEvent, hence it the highlight of actions isn't triggered sometimes.

I haven't seen this before, is this issue happens on specific DCC ?

  • mouse release triggers action where cursor currently is rather than the on where it originally clicked

Hmmm, the behavior was referenced from Maya's menu, and the rule that I applied was "trigger what's been highlighted". Shouldn't it triggers the thing that is currently on top of ?

@iLLiCiTiT
Copy link
Copy Markdown
Contributor Author

iLLiCiTiT commented Jan 30, 2020

I haven't seen this before, is this issue happens on specific DCC ?

Each host has same issue, at least: Maya, Nuke.
image

Hmmm, the behavior was referenced from Maya's menu, and the rule that I applied was "trigger what's been highlighted". Shouldn't it triggers the thing that is currently on top of ?

Didn't realize but you have got true, it is default at all apps. Maybe it was more user friendly in my point of view. Can be easily changed back...

@davidlatwe
Copy link
Copy Markdown
Collaborator

Ah, for the first issue, I think I've get what you're saying.
The mouse movement looks like pretty laggy, see below.

before

But the solution in this PR (removing OptionMenu) will break the highlighting after mouse is pressed and holded, see the comment in #482 👉 here.

Having that said, I have had take a deep look into this and I found the cause was action widget's label did not set mouseTracking to True ! And that's why it appears laggy, since there's a big mouse movement dead zone (the rect of label widget) in each action widget !

So just adding label.setMouseTracking(True) into OptionalActionWidget.__init__ would solve the issue, see below.

after

🎉

@tokejepsen
Copy link
Copy Markdown
Collaborator

Looks promising @davidlatwe !

@davidlatwe
Copy link
Copy Markdown
Collaborator

Thanks @tokejepsen , see if @iLLiCiTiT could confirm what I have found. :)

@iLLiCiTiT
Copy link
Copy Markdown
Contributor Author

iLLiCiTiT commented Jan 30, 2020

see if @iLLiCiTiT could confirm what I have found. :)

@davidlatwe I can confirm it works :)

But to be hones I don't like that mouse event (highlighting) is handled in parent item instead of item itself, because OptionalAction is not usable in QMenu (which may be handy in future), but the true is, nobody will touch this code for a long time so I believe it doesn't matter.

...will break the highlighting after mouse is pressed and holded ...

Qt is fully customizable if you know how :) (I don't know at this moment but it won't be a problem to try)

Now question is if I should cancel this PR and create new (or you'll create new)?

@davidlatwe
Copy link
Copy Markdown
Collaborator

But to be hones I don't like that mouse event (highlighting) is handled in parent item instead of item itself

I don't like it, too ! But it was painful to find the way to poperly implement the desired behavior from each item. 🤕

Qt is fully customizable if you know how :)

Agree. 🛠

Now question is if I should cancel this PR and create new (or you'll create new)?

I think I would close this one and reference it in the new PR. And would be great if you could submit a new one. :)

@iLLiCiTiT
Copy link
Copy Markdown
Contributor Author

Ok, I let this PR open for now, to keep it in my mind, and will look if is possible to implement mouse events in OptionalActionWidget with same mouse press/release events like now. If I won't find it today I'll create new PR this evening with your solution.

@iLLiCiTiT
Copy link
Copy Markdown
Contributor Author

I had a little bit of free time for exploration and result is that it is possible, but only with overriding QMenu.
Reason is that QMenu by default send mouse move events only to pressed action. So to be able change highlight of hovered actions after mouse press it is necessary to override QMenu to trigger mouse move events for all actions.
I would prefer to override QMenu and highlighting changes let for action widget but that is time consuming stuff so I'll create PR with your solution :)

With this change:

  • OptionBox was changed from QWidget to QLabel since it's only child was QLabel

@iLLiCiTiT iLLiCiTiT mentioned this pull request Jan 30, 2020
@iLLiCiTiT
Copy link
Copy Markdown
Contributor Author

moved to #521

@iLLiCiTiT iLLiCiTiT closed this Jan 30, 2020
@iLLiCiTiT iLLiCiTiT deleted the highlight_update_fix branch January 30, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants