Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Publisher: context menu option to revert each attribute to default #147

Conversation

bradenjennings
Copy link
Contributor

Changelog Description

Publisher - now has a "Revert to default" context menu option for each menu of various widgets ( Int, Float, String and Enum)

Additional info

Paragraphs of text giving context of additional technical information or code examples.

Testing notes:

  1. start with this step
  2. follow this step

@ynbot ynbot added size/XS type: feature Adding something new and exciting to the product labels Mar 6, 2024
@bradenjennings
Copy link
Contributor Author

bradenjennings commented Mar 6, 2024

Revert to default menu options now appears for Bool, Int, Float, String and Enum menus.

It adds itself to existing menus, instead of replacing them.

image

@bradenjennings bradenjennings changed the title feature/AY-3430_revert_to_default_context_menu_option Publisher - context menu option to revert each attribute to default Mar 7, 2024
@bradenjennings bradenjennings changed the title Publisher - context menu option to revert each attribute to default Publisher: context menu option to revert each attribute to default Mar 7, 2024
@jakubjezek001
Copy link
Member

jakubjezek001 commented Mar 7, 2024

image
BlackMagic Resolve

Not sure how much more complexity it would add, but this approach seems to be best UX. Reset button on right of each row.

@tokejepsen
Copy link
Member

Maybe @Innders could chip in with ideas or whether there is some convention in AYON already.

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 7, 2024

image BlackMagic Resolve

Not sure how much more complexity it would add, but this approach seems to be best UX. Reset button on right of each row.

qargparse actually does that and thus the OpenPype Loader Option box fields also do that, e.g. see the button to right of Remove Publish Folder here:

image

There does seem to be a bug though that if you click that reset button it doesn't disappear in that particular UI - but it's a decent reference.

@iLLiCiTiT
Copy link
Member

I do agree that right click is not good option to reset value, consideting I want to reset all attributes to default state.

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 8, 2024

I do agree that right click is not good option to reset value, consideting I want to reset all attributes to default state.

Well, not always. I usually want to revert an attribute or two - not the full instance. But if I'd want to revert 10, then yes. This will be very slow. However, the context menu could show a "shortcut" label like e.g. CTRL+LMB and when clicking that on an attribute that it reverts to default. Works perfectly fine like that in Houdini.

@bradenjennings
Copy link
Contributor Author

There is a separate ticket for a reset all attributes button.
So in this case you wouldn't need to click every attribute directly.
#153

What's the consensus should we keep the context menu, or add a button next to each attribute.
Personally I find its a lot of visual noise having a button next to each attribute.

@bradenjennings
Copy link
Contributor Author

I find the Houdini context menu option to be a clean non distacting implementation.

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Mar 11, 2024

I find the Houdini context menu option to be a clean non distacting implementation.

You just described hidden feature, that requires 2 clicks. But I'm not known for UX, so hard to tell.

EDITED:
@mkolar mentioned that if we'll add the button then right click should be enough.

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 11, 2024

I find the Houdini context menu option to be a clean non distacting implementation.

I personally quite like Houdini's implementation - only if we can also match the shortcut behavior of CTRL + MMB to revert to default and the context menu label showing the shortcut.

image

That way you can also perform the revert in one click per attribute.

Comment on lines +333 to +336
action.triggered.connect(self.reset_to_default_requested)
menu.insertAction(first_action, action)

def reset_to_default_requested(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method name is not ideal. It should be something like set_to_default_value, or private method for signal handling and in that case _on_default_value_requested.

self.setContextMenuPolicy(QtCore.Qt.CustomContextMenu)
self.customContextMenuRequested.connect(self._on_context_menu)

def _on_context_menu(self, pos):
Copy link
Member

@iLLiCiTiT iLLiCiTiT Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Menu and action does not have to be stored under self they live and die during stack of method. The same for other widgets.

self._multivalue
and obj is self._input_widget
and event.type() == QtCore.QEvent.FocusOut
):
self._set_multiselection_visible(True)
return False

def _on_menu_timeout(self):
menu = self.findChild(QtWidgets.QMenu, "qt_edit_menu")
Copy link
Member

@iLLiCiTiT iLLiCiTiT Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if all DCCs are using this name of menu, considering Houdini, Nuke, Maya and SubstancePainter etc. Could someone validate those DCCs if the right click does work for text and number fields there?

We might need to use createStandardContextMenu if it does not work there.

@iLLiCiTiT
Copy link
Member

That way you can also perform the revert in one click per attribute.

So, let it in context menu with hint to use Ctrl + MMB instead (oh, Mac users will be pissed 😈 ).

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 11, 2024

(oh, Mac users will be pissed 😈 ).

Would be Command + MMB then I suppose. No idea how that works. Didn't Qt have something built-in for that and also to labelize menu entries, etc - by QAction.setShortcut and enabling its visibility in the context menu.

Also according to Qt docs:

Note: On Mac OS X, the CTRL value corresponds to the Command keys on the Macintosh keyboard, and the META value corresponds to the Control keys.

As such, that should handle itself?

@iLLiCiTiT
Copy link
Member

Would be Command + MMB then I suppose.

It was not about CTRL, you would have to find MMB :)

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 11, 2024

It was not about CTRL, you would have to find MMB :)

🤦‍♂️ Of course. Anyway, not sure what Houdini does there.

@mkolar
Copy link
Member

mkolar commented May 3, 2024

Seriously folks. Context menu is more than enough here. We're talking about something that is probably used very occasionally and it's super common to add these kinds of things to right click. It's a few attributes, so resetting all at once is really on a fringe of usefulness compared to the effort.

@iLLiCiTiT iLLiCiTiT marked this pull request as draft August 13, 2024 09:28
@ynbot
Copy link
Contributor

ynbot commented Aug 13, 2024

@iLLiCiTiT
Copy link
Member

Closed in favor of #986

@iLLiCiTiT iLLiCiTiT closed this Nov 5, 2024
@iLLiCiTiT iLLiCiTiT removed their assignment Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS type: feature Adding something new and exciting to the product
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants