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: show visual difference default vs overridden attributes #146

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

bradenjennings
Copy link
Contributor

Changelog Description

With publisher attributes, there is no way to know if artist changed value on them from default.
This could be easily shown just by changing font-face or color.
Please see this ticket for more details: https://app.clickup.com/t/6658547/AY-2614

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 type: enhancement Improvement of existing functionality or minor addition size/XS labels Mar 5, 2024
@bradenjennings
Copy link
Contributor Author

Here is an example image of the implementation, which makes overridden attributes bold.
image

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 5, 2024

Here is an example image of the implementation, which makes overridden attributes bold. image

With this making the label bold instead of the value (like Houdini does) I have the feeling that maybe it's not as clear, in particular because it makes the label look more like a header/important label than just a non-default state.

@bradenjennings bradenjennings changed the title enhancement/AY-2614_display_overriden_vs_default_attributes_in_publisher Publisher - show visual difference default vs overridden attributes Mar 5, 2024
@bradenjennings
Copy link
Contributor Author

With this making the label bold instead of the value (like Houdini does) I have the feeling that maybe it's not as clear, in particular because it makes the label look more like a header/important label than just a non-default state.

There is boolean fields, which just show a slider. If we apply bold to the widget (instead of label).
How would these boolean widgets show they have been overridden.

Bolding the label seems like the most consistent easy way to go.

@m-u-r-p-h-y
Copy link
Member

With this making the label bold instead of the value (like Houdini does) I have the feeling that maybe it's not as clear, in particular because it makes the label look more like a header/important label than just a non-default state.

There is boolean fields, which just show a slider. If we apply bold to the widget (instead of label). How would these boolean widgets show they have been overridden.

Bolding the label seems like the most consistent easy way to go.

we are using different color coding for overrides throughout the system. https://ayon.ynput.io/docs/admin_settings/#categories

@bradenjennings bradenjennings changed the title Publisher - show visual difference default vs overridden attributes Publisher: show visual difference default vs overridden attributes Mar 7, 2024
@jakubjezek001
Copy link
Member

we are using different color coding for overrides throughout the system. ayon.ynput.io/docs/admin_settings/#categories

In this case I am assuming it would be blue color. Since that is signalizing unsaved changes - which is closest you can get. Other is Studio override - green, Project override - orange.

@tokejepsen
Copy link
Member

@m-u-r-p-h-y had any color in mind?
As @jakubjezek001 is saying there is not really any color scheme for this kind of override that would fit this. Since the PR is for overrides from default only, then using bold font seems like the best option.

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 7, 2024

Gave this a quick test run. First off, really great seeing work being done on this.

Here are my findings:

  • The label turning bold is quite annoying for the entry that has the biggest label since things "shift" around because the label field gets wider - which isn't preferred since e.g. it makes it hard to 'toggle' things if you'd want to. Even though I think the feature itself is so useful that we might just want to first implement it in the less aesthetic way if we can't think of anything better. I do like the color coding that was mentioned so that might be something to give a go.

The bugs:

  • The 'nondefault' state does not trigger if you change a value from non-default, save, then reset publisher.
  • The 'nondefault' state does not update when clicking between two instances that have different values for the same attribute. E.g. create two instances, set one value to be non-default on of them. No toggle between them, state doesn't follow along as it shoudl.
  • The 'nondefault' state also does not trigger in the 'multiselection' state.

With the bugs fixed I think this is already going to be really really nice! - Especially if combined with the "revert to default" context option PR.

@LiborBatek
Copy link
Member

Maybe not relevant but how about Instead of changing the label change the way the value itself being shown?

It would look quite neat imho and clean too if those altered non default values would have been displayed in different color or in italics or bold etc.

Screenshot 2024-03-08 165159

Not sure if doable tho...

@tokejepsen
Copy link
Member

tokejepsen commented Mar 8, 2024

Maybe not relevant but how about Instead of changing the label change the way the value itself being shown?

Think that might get tricky when dealing with non-inputs like checkboxes and booleans. There is usually always a label to an attribute which is all the text based.

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 8, 2024

Maybe not relevant but how about Instead of changing the label change the way the value itself being shown?

Think that might get tricky when dealing with non-inputs like checkboxes and booleans. There is usually always a label to an attribute which is all the text based.

Checked Houdini quickly - but they are sneaky! Checkboxes have their labels on the right hand side.

image

Braden Jennings added 2 commits March 11, 2024 09:39
@bradenjennings
Copy link
Contributor Author

  • The 'nondefault' state does not trigger if you change a value from non-default, save, then reset publisher.
  • The 'nondefault' state does not update when clicking between two instances that have different values for the same attribute. E.g. create two instances, set one value to be non-default on of them. No toggle between them, state doesn't follow along as it shoudl.
  • The 'nondefault' state also does not trigger in the 'multiselection' state.

I think I've fixed all these bugs now and pushed the latest code.

@bradenjennings
Copy link
Contributor Author

bradenjennings commented Mar 10, 2024

Here is multi selection mode with visual indicator one of the attributes have been overridden.

NOTE: I'm now using colour to show visual difference, rather than making the font bold.
image

Comment on lines 1640 to 1643
if is_default:
label_widget.setStyleSheet("")
else:
label_widget.setStyleSheet("color: #4287f5")
Copy link
Collaborator

@BigRoy BigRoy Mar 11, 2024

Choose a reason for hiding this comment

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

Looking at the code duplication of this stylesheet value I wonder if we should instead implement this:

def set_styled_property(widget, property, value):
    """Set widget property and refresh dynamic style"""
    if widget.property(property) == value:
        return
    widget.setProperty(property, value)
    # force stylesheet refresh
    style = widget.style()
    style.unpolish(widget)
    style.polish(widget)

According to what I read online that should work with e.g. styling as:

from PySide2 import QtWidgets, QtCore

app = QtWidgets.QApplication.instance()
app.setStyleSheet("*[nondefault=true] { font: bold }")


button = QtWidgets.QPushButton("hello")
button.show()

timer = QtCore.QTimer()
timer.setInterval(1000)


def set_styled_property(widget, property, value):
    """Set widget property and refresh dynamic style"""
    if widget.property(property) == value:
        return
    widget.setProperty(property, value)
    # force stylesheet refresh
    style = widget.style()
    style.unpolish(widget)
    style.polish(widget)

def toggle_property():
    new_state = not bool(button.property("nondefault"))
    print(f"Setting state: {new_state}")
    set_styled_property(button, "nondefault", new_state)

timer.timeout.connect(toggle_property)
timer.start()

This shows that it does work, even if the stylesheet itself exists globally and not on the object itself. (Note that you usually don't want to apply it on the application, but this is just example code).

By doing it this way we can adjust the more global AYON stylesheet to have e.g.

#PublishWindow *[nondefault=true] { font: bold }

@m-u-r-p-h-y
Copy link
Member

I find it works really well in Ayon -> Project Editor

It is nice, tidy, and super clear what is inherited and what is overridden

image

Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

It works nicely, highlighting any non default values via orange color typing...

Screenshot 2024-05-17 150919

But for some reason it wont revert back to white typing on Review and Resolution attributes. For the rest occasions seems working normally so whenever user puts the value back to its defaults aka DB entry, it reverts to normal white typo.

Screenshot 2024-05-17 152624

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented May 17, 2024

But for some reason it wont revert back to white typing on Review and Resolution attributes. For the rest occasions seems working normally so whenever user puts the value back to its defaults aka DB entry, it reverts to normal white typo.

I'm afraid this is something that either must be resolved by the create plugin in different PR, or something we have to live with.

EDITED:
The create plugin does set default value to 0 and based on the logic I guess it can't change because it is based on product name filtering, so there is not much to do.

@LiborBatek
Copy link
Member

@m-u-r-p-h-y @antirotor any opinions on that ( review shown as overriden if not set to zero value for resolution ) so most of the times shown as orange override no matter what user set/not

@iLLiCiTiT
Copy link
Member

This actually uncovered possible issues in PR #153 . Both PRs should probably wait until we have attribute definitions per instance.

@BigRoy
Copy link
Collaborator

BigRoy commented Jun 18, 2024

I'd still love this feature (and #153!) and think "living with it" for now until we have a better idea on how to avoid the odd behavior in edge cases may be worth it. It may also highlight that we may need better defaults somewhere, etc. and maybe forces us down the rabbit hole.

@dee-ynput @antirotor (or maybe @mkolar) any inputs regarding this from a product or pipeline perspective?

Both PRs should probably wait until we have attribute definitions per instance.

Or do we indeed feel that maybe pushing this on the priority list may be worth it more beforehand?

@dee-ynput
Copy link

any inputs regarding this from a product or pipeline perspective?

@iLLiCiTiT worrying sounds legit, and it definitely deserves a dive into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS type: enhancement Improvement of existing functionality or minor addition
Projects
Status: Change Requested
Development

Successfully merging this pull request may close these issues.

9 participants