getavalon / core

The safe post-production pipeline - https://getavalon.github.io/2.0
MIT License
214 stars 48 forks source link

Fix QPushButton::menu-indicator being cut off #450

Closed BigRoy closed 4 years ago

BigRoy commented 4 years ago

Issue

The Instance Creator's subset dropdown button had it's "triangle" menu indicator cut off on the right hand side. This made it look very confusing and hard to understand. It was clearly wrong, see:

triangle

Menu Indicator cut off on right-hand side

This was due to the QPushButton::menu-indicator styling in Avalon's stylesheet.

This was reproducable on just a QPushButton example:

# Assumes QApplication already runs
from avalon.vendor.Qt import QtWidgets
from avalon import style

box = QtWidgets.QPushButton("Test")
box.setStyleSheet(style.load_stylesheet())
menu = QtWidgets.QMenu(box)
menu.addAction("test")
box.setMenu(menu)
box.show()

Output:

Menu Indicator vertical alignment

Aside of that the vertical alignment of the triangle was also slightly odd, as it was forced to the bottom right. It's more expected to have this vertically centered on the button on the right-hand side.

Comparison:

What's changed?

The stylesheet is updated to not have the indicator cut off and have it vertically centered on right hand side.


Reference:

iLLiCiTiT commented 4 years ago

that's huge triangle compared to text

BigRoy commented 4 years ago

that's huge triangle compared to text

Interesting. It didn't feel so off to me. But now comparing with other applications it does seem bigger than what's usually the case. Should we try and fix that up too? Anyone cares to have an opinion? :)

Ok. Can't unsee this. It's definitely too large. And actually in the Creator it still feels off too. Also just found out the disabled state is still wrong too. Investigating..

iLLiCiTiT commented 4 years ago

you can try to add width and height to stylesheet (10px is random value):

QPushButton::menu-indicator  {
    width: 10px;
    height: 10px;
    subcontrol-position: right;
}

EDITED: Or maybe in next PR? :)

davidlatwe commented 4 years ago

I'd say we moving on for now :rocket: Since that sounds like an issue that can differ in each host/environment.

BigRoy commented 4 years ago

I'd say we moving on for now

Unfortunately it does look quite off in the Creator too. See:

not_centered

Not as bad as before, but still not super clean. Also, the disabled state is still a problem:

disabled

Will look into it now.

iLLiCiTiT commented 4 years ago

override QPushButton::menu-indicator:disabled should work I think

BigRoy commented 4 years ago

This PR came to existence to it feeling off in the Creator. So I made sure it looks good there now, which includes a small tweak to the tool to improve styling even more.

The change to Creator make sure the QPushButton's height aligns with the line edit (it wasn't before somehow) plus it pushes the button and line edit slightly closer to each other (less spacing) so that they appear more related.

Anyway, here is the result

Creator Enabled enabled

Disabled disabled2

QPushButton big Enabled enabled_big

Disabled disabled2_big

Somehow the disabled state icon looks slightly blurry, I'm not sure why that is.

davidlatwe commented 4 years ago

Changing to

QPushButton::menu-indicator  {
    subcontrol-origin: padding;
    subcontrol-position: right;
    width: 8px;
    height: 8px;
    padding-right: 5px;
}

QPushButton::menu-indicator:disabled {
    image: url(:/qss_icons/rc/down_arrow_disabled.png);
    width: 9px;
    height: 9px;
    padding-right: 3px;
}

Seems better creator

BigRoy commented 4 years ago

Thanks @davidlatwe that indeed works much better, also much neater when toggling the state because now it doesn't seem to "jump around". 👍 Fixed with https://github.com/getavalon/core/pull/450/commits/9288d0b9266f2086efa92168acd037752e55c792

Feel free to merge.