openDsh / dash

Join us on Slack! https://join.slack.com/t/opendsh/shared_invite/zt-la398uly-a6eMH5ttEQhbtE6asVKx4Q
GNU General Public License v3.0
238 stars 69 forks source link

Now showing colored AA icon when AA is connected #68

Closed stefan-sherwood closed 2 years ago

stefan-sherwood commented 3 years ago

Not connected (light mode) image

Connected (light mode) image

Connected (dark mode) image

rsjudka commented 3 years ago

shouldn't the AA logo be blue and white?

or is dash doing some masking that's messing with the original?

rsjudka commented 3 years ago

im not a big fan of changing the icons like that (not sure if you've tested on a pi but dynamically colorizing icons also causes some lag - which is why I only really use it when the user changes the theme color)

might be better to use the On and Off states for the icon?

it might be better to not even use those *ize helper functions since this is super specific to the AA icon

stefan-sherwood commented 3 years ago

I took the official AA logo and somewhat darkened the gray part so that it worked well with both the light and dark scheme. Otherwise it got washed out with the light theme.

stefan-sherwood commented 3 years ago

Changing the icon with a SetIcon() call is very fast and connection/disconnection is a rare event. Have you actually experienced a lag? I can time it but my instinct is that it is extremely quick.

I considered changing the Colorize() code, which is uses legacy calls to instead use QGraphicsColorizeEffect. But it is already so fast that it doesn't seem worth it.

The icon changes color on connection in every consumer AA system I've seen.

stefan-sherwood commented 3 years ago

shouldn't the AA logo be blue and white?

Yes, it should be. Dash is making it monochromatic with the Colorize() function.

stefan-sherwood commented 3 years ago

set_icon() for connected icon (not colorized): 1 millisecond set_icon() for colorized icon (w/creating masks and all that jazz): 63 milliseconds

Note: we were previously making this second call lots of times on theme switch, which is the primary reason it was so slow

rsjudka commented 3 years ago

can we add an option to toggle this feature?

stefan-sherwood commented 3 years ago

can we add an option to toggle this feature?

Yes, certainly

stefan-sherwood commented 3 years ago

I debated various implementations of this but in the end this was the simplest. IMO the coolest was setting a stylesheet and then just toggling a property ("connected") of the button.

For posterity, this was the call:

oaButton->setStyleSheet( QString("QPushButton[connected=true]{qproperty-icon: url(\":/icons/%1.svg\");} ").arg(icon_name) );

and this was the secret sauce to activate it

connect(oaPage, &OpenAutoPage::connected, this, [this, oaButton](bool connected) {
   oaButton->setProperty( "connected", connected );
   oaButton->style()->unpolish(oaButton); oaButton->style()->polish(oaButton);
}
rsjudka commented 3 years ago

I considered changing the Colorize() code, which is uses legacy calls to instead use QGraphicsColorizeEffect. But it is already so fast that it doesn't seem worth it.

i was having some issues using QGraphicsColorizeEffect (pixelated the icons too much) so I chose to use the masking

oh yeah if you can tell I try avoiding modifying the stylesheet as much as I could (since its already doing some funny stuff lol)

stefan-sherwood commented 3 years ago

image

rsjudka commented 3 years ago

ill wait to hear back from others who plan on testing out this fork, but look good to merge 🙂

robert5974 commented 3 years ago

I'll test it soon. Maybe tonight if I get a chance.

Robert Crowley

On Thu, Jun 3, 2021, 8:04 PM Robert Stanley Judka @.***> wrote:

ill wait to hear back from others who plan on testing out this fork, but look good to merge 🙂

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openDsh/dash/pull/68#issuecomment-854261785, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLUDVGH5VINEIGOZXTZWQDTRAKCRANCNFSM45KAY64A .

egisz commented 2 years ago

@stefan-sherwood, I use dark theme + orange color (matched to BMW interior lights) and blue connected icon looks a bit odd :( Maybe we could use selected color to indicate AA is connected instead of constant blue?

Disconnected: aa-not-connected

Connected: aa-connected

stefan-sherwood commented 2 years ago

The color of the icon is the standard blue/gray Android Auto icon. I'm open to other ideas but presently the selected tab icon gets the color of the theme color to indicate which page you're on.