Page MenuHome

Fix T80356: Icon alpha theme property affects previews
Needs RevisionPublic

Authored by Hans Goudey (HooglyBoogly) on Wed, Sep 2, 8:48 PM.

Details

Summary

Check the icon's type before using the icon_alpha theme property.

Diff Detail

Repository
rB Blender
Branch
icon-alpha-no-previews (branched from master)
Build Status
Buildable 9944
Build 9944: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Wed, Sep 2, 8:48 PM
Hans Goudey (HooglyBoogly) created this revision.
This revision is now accepted and ready to land.Thu, Sep 3, 10:43 AM

What if it is the custom UI icon? Like here T78630, T64628 for example.

Julian Eisel (Severin) requested changes to this revision.Thu, Sep 3, 3:21 PM

Custom icons are indeed an issue. Matcaps and such use ICON_TYPE_BUFFER, other previews like material previews use ICON_TYPE_PREVIEW. However ICON_TYPE_PREVIEW is also used for custom icons (should maybe be mentioned in a comment at its definition). So we can't just rely on that currently.
I think there could just be a bool use_theme_icon_alpha passed to icon_create() and stored in Icon. That gives better control and seems generally a bit nicer, since the option is controlled at a higher level.

Another issue is that custom icons are used for both, regular icons, as well as custom previews. So we'd need the information on how the icon is used to know if we should apply the alpha. For now I'd say just always apply the alpha for custom icons. If needed we can let the Python API have more control.

source/blender/editors/interface/interface_icons.c
1783

Event icons may be mixed with other icons, so we should apply alpha for them too, otherwise:


Note that this is already an issue in master, but that's separate. So I'd just add ICON_TYPE_EVENT here.

This revision now requires changes to proceed.Thu, Sep 3, 3:21 PM

Do we even need this option? This was for color icons ("to reduce contrast"), now the color of icons respects the theme settings.

Removing it would be fine with me. I also don't see a need and initially thought it was added during 2.8. But seems it's an old option and probably redundant by now.

What about just doing that? The setting seems pretty useless to me too. IMO there should be better ways to theme the icons, but I don't see why this needs to be one of them.

What about just doing that? The setting seems pretty useless to me too. IMO there should be better ways to theme the icons, but I don't see why this needs to be one of them.

That wouldn't be the end of the world... but I actually like that feature...
I got a quite dark custom theme and (depending on the mood hehehe) sometimes I feel like the icons are in the way.... so be able to adjust the opacity of all the icons at same time with just one slider was kinda neat....

Do we even need this option? This was for color icons ("to reduce contrast"), now the color of icons respects the theme settings.

Removing it would be fine with me. I also don't see a need and initially thought it was added during 2.8. But seems it's an old option and probably redundant by now.

What about just doing that? The setting seems pretty useless to me too. IMO there should be better ways to theme the icons, but I don't see why this needs to be one of them.

I will leave the final word on this up to @Brecht Van Lommel (brecht), he added this option years ago (rB7217518179d9ed).

I'm not particularly attached to keeping the feature, it's not important. It's still convenient somewhat, so we might as well keep it and not break API/theme compatibility?

However ICON_TYPE_PREVIEW is also used for custom icons (should maybe be mentioned in a comment at its definition). So we can't just rely on that currently.

I'm not sure how deep this goes, but using a different ICON_TYPE for custom icons would be good then, since using ICON_TYPE_PREVIEW makes little sense.