Do theming better

[eteran]

Right now our theming support is "good enough", but it's not great.

We still depend on the host desktop to tell us what colors to use in a LOT of places. So I've got an idea to do this better!

1. We ALREADY have code in main.cpp to detect well enough if we are using a desktop with dark mode/light mode
2. We should have (at least) 2 "baked in" color themes "light" and "dark" which cover EVERYTHING. They should not only set the disassembly and similar colors, they should set the style sheets for all widgets as best we can. (We can probably dump what KDE offers color-wise into style sheets and use them as a baseline).
3. If we detect that the user has a dark mode desktop on startup, we can offer to switch to a built-in dark mode theme (ask only the first time we detect this)
4. we can make a config option to manually set the theme based on a what theme files it finds in the config directory

[10110111]

They should not only set the disassembly and similar colors, they should set the style sheets for all widgets as best we can

But please make this alteration of normal widgets optional. Style sheets are quite an ugly way to theme, and can easily interfere with theming engines. I'd definitely prefer KDE's palette settings for my desktop (which are richer than QPalette) instead of in-app stylesheeting.

[eteran]

I'm gonna do my best to make things look good! So yea, everything will be optional.

[eteran]

@10110111 Since you rightly point out that style sheets can ruin the actual widget theming of things like KDE. If we set the QApplication::palette nice and early, it filters down to all the child widgets (unless they override a palette of course).

So what we'll do is use the same theming concept from the INI file (where the user can specify colors) and set as much of the app palette as we need to, right in main().

And of course, we'll default to a "System" theme, which just inherits what the system uses.

I'm not sure how to portably use KDE's palette settings over just a plain QPalette ... but I'm not 100% sure we need everything those palette settings offer (unless I'm missing something obvious)

[10110111]

I'm not sure how to portably use KDE's palette settings over just a plain QPalette

This is unneeded. It seems KDE's palette mostly just adds hover/focus colors and titlebar colors. All other categories seem to be available in QPalette, which does get set up by KDE libraries from KPalette (which is used by KDE's theming engines). So you can just use QPalette, ignoring whatever settings files KDE may have.

[eteran]

OK, I think I have a handle on how I want to do palette, it's not too bad code wise. in the .config/codef00.com/ we'll have a themes directory. Which will have INI files of the same style as our current theme support. (Will merge it with two resource files that represent built-in themes).

edb will default to a "System" theme, which is no color overriding at all, but offer in the config the ability to select on of the themes in the directory.

On the off chance that a user is use old style themes, we can just silently migrate them to the new scheme by saving their settings to a new file in .config/codef00.com/

[eteran]

@10110111 So just as an update, using Qt's QPalette stuff, I can get things 99.9% of the way there. The only thing I haven't been able to figure out is a small detail, but I know it's doable because Qt Creator seems to pull it off (sadly, it is such a complex beast I don't know HOW it pulls it off).

Basically, it looks nearly perfect, except for "standard dialog icons". So if I set my dark theme, and then choose "Open", a standard file dialog box comes up looking nice and dark themed... but the navigation icons and some of the mime-type icons are still colored as if they are for a lighter theme.

The simplest example is that the Cancel button still has a black icon, instead of a white one. So... I'm close.

If you happen to know a trick to pull this off, I'm all ears, but until then I will be diving into the code of applications that seem to get it right to see if I can figure it out!

[eteran]

[10110111]

How do you set up dark theme in Qt Creator to get this automatic icon color change?

[10110111]

I suppose you just choose a Dark scheme in Qt Creator's own menus, rather than change the system colors. This thing seems to work on the level of their own recoloring code hidden somewhere in src/libs/utils/icon.cpp, and is related to the IconsBaseColor and friends you can grep in the source. Then these colors are written in the scheme files like share/qtcreator/themes/dark.creatortheme.

Disclaimer: I've not dived into this code, so I might be wrong. But at least it seems to me that Qt Creator's icons won't work well without its own theme files.

[eteran]

So, yes, you just choose it in the settings and restart the app, and you get the new color changes activated:

Anyway, I've come to similar conclusions. But they get the right result even for things like:

    auto buttonBox = new QDialogButtonBox;
    buttonBox->addButton(QDialogButtonBox::Cancel);

When they do that, the Cancel button has a icon suitable for dark mode, but edb doesn't get one. So they MUST be doing some API call to tell Qt (or KDE) to use an alternate coloration for standard icons.

Because as far as I can see, that standard Icon in Qt comes from an instance of QStyle which I haven't seen QtCreator do much icon related in.

[eteran]

So I figured out the issue (i think).

basically, I've been doing the palette stuff right. I literally compiled creator from source, and disabled/enabled code until I found out what was letting it choose the right icons... It boils down to QApplication::setPalette(p); ...

So why doesn't it work for edb then? Well It's how we do our build in icon theming!

Do choose dark vs light icons, we do the following:

    if (qApp->palette().window().color().lightnessF() >= 0.5) {
        QIcon::setThemeName(QLatin1String("breeze-edb"));
    } else {
        QIcon::setThemeName(QLatin1String("breeze-dark-edb"));
    }

Which is of course, not the breeze or breeze-dark themes. So which is going on?

Well, when a QIcon::fromTheme call is made, it will first try to find it in the theme, and if not found, will try to fallback on a system one. It seems that this fallback doesn't do dark/light switching, it just finds what it finds (for me the fallback is breeze, but not breeze-dark and then it's done searching.

So I think to fix this properly, we HAVE to switch to >= Qt 5.6 as a minimum version because that was the first version to support multiple theme search paths which is the "right way" to support your own icons.

So, I'm going to make the call to say "we require 5.6" now. We'll jump to a minimum version of 5.9 when we do the switch to C++17.

[10110111]

What exactly API do we want from Qt ≥ 5.6?

[eteran]

Specifically, if we use this, I believe we can do our internal icon themeing "more properly" and in such a way that doesn't block the normal icon theme lookup in this corner case.

https://doc.qt.io/qt-5/qicon.html#setThemeSearchPaths

[10110111]

What does QIcon::themeSearchPaths() return for you?

[eteran]

Oh shit, wait. I misread it. That was introduced in 4.6! LOL.

[10110111]

I'm not actually sure this function would help. It just lets you set theme search paths, and if a theme is found without an icon, we're still going to the fallback theme, regardless of the paths (unless this theme is not found, of course, but then you'll not get any icon at all, I suppose).

Anyway, let's see what your tests give :)

[eteran]

OK, well assuming that the setThemeSearchPaths works as I believe it will. Do you think we should skip the 5.6 Qt upgrade and stick with the current 5.2 until we NEED to upgrade it?

[10110111]

Do you think we should skip the 5.6 Qt upgrade and stick with the current 5.2 until we NEED to upgrade it?

If you're going to make a minor release, I think it's better to keep the required versions down (so that the distributions could simply upgrade). But if the next release is supposed to be a major one, then there are all the reasons to upgrade the requirements.

[eteran]

I think it might work because it's what QtCreator does and it works there. Also, there is seemingly a difference between "it was found in one of the search paths" and we didn't find the icon in the theme itself, so we're gonna fallback on the desktop icon settings (my desktop is set to a non-dark UI currently). Which is why I think it may work... maybe.

[eteran]

Hmm, I it's possible that THIS is what we really want, but was unfortunately introduced in 5.11, which is just too new:

https://doc.qt.io/qt-5/qicon.html#setFallbackSearchPaths

Will continue to experiement.

[10110111]

Hmm, this sounds just what we need. But why not simply #if the support for this, and let some icons be wrong if the users' Qt is too old?

[10110111]

In fact, I see no real reason to avoid the requirement of a high Qt version in a new major release. The distributions won't package it for any already-released version, and if the users really want it, they'll install all the required prerequisites.

For Ubuntu, there's even a PPA with new Qt versions. How about Qt 5.15.0 for Xenial? ;)

[eteran]

Yea, I think you may be right about just doing it with #ifdefs for now.

Though I'm not 100% sure what you mean by this because of the double negative.

In fact, I see no real reason to avoid the requirement of a high Qt version in a new major release

Are you suggesting that we bump the minimum reqs or wait to do it? Or never do it and just ifdef indefinitely? LOL

[10110111]

because of the double negative.

"No reason to avoid X" means "no reason to wait until !X". There's really no reason to wait for a version of Qt to stop feeling too new. We can easily bump the requirements. Maybe not to the latest version—to ease the life of the users of latest distributions like Ubuntu 20.04—but at least to something reasonable, like Qt 5.12.

[eteran]

LOL, sorry didn't mean to misinterpret what you said 😜.

I generally agree with you about reqs, though I like to be as I'm sure you'll be noticed a bit conservative with them to make it as easy on users as possible even if they are running older systems.

I think the oldest still supported LTS whatever that may be, is a good target for the next versions release we do.

[eteran]

Arrrgggg..

I'm experimenting with code like this:

    // Light/Dark icons on all platforms
    if (qApp->palette().window().color().lightnessF() >= 0.5) {
#if QT_VERSION >= QT_VERSION_CHECK(5, 11, 0)
        QIcon::setFallbackSearchPaths(QIcon::fallbackSearchPaths() << ":/icons/breeze-edb");
#else
        QIcon::setThemeName(QLatin1String("breeze-edb"));
#endif
    } else {
#if QT_VERSION >= QT_VERSION_CHECK(5, 11, 0)
        QIcon::setFallbackSearchPaths(QIcon::fallbackSearchPaths() << ":/icons/breeze-dark-edb");
#else
        QIcon::setThemeName(QLatin1String("breeze-dark-edb"));
#endif
    }

Which looks reasonable to me :-P. it seems that Qt is selectively choosing to invert my icons though! even when I force an icon path that only contains grey on transparent icons, it is choosing to render them as black. For SOME color schemes it does the right thing, for others, it is seemingly inverting my colors.

Ironically, when I try normal icons on a dark theme, it never inverts them. So I simply can't get it to do the right thing apparently, LOL.

[eteran]

@10110111 OK, I think I have it all sorted out.

Here's a branch with the basis for implementation, please let me know what you think:

https://github.com/eteran/edb-debugger/pull/784

Eventually, I want to move the themes out of the edb.conf file and into separate theme files so they can be more easily shared and selected from the UI). Any colors not set, should just take on the system defaults.

[eteran]

The only thing I'm still pondering if how the text formatting coloring should interact with this. What I mean is, if we keep the formatting of disassembly and hexviews separate from application theming, they can obviously clash.

Basically the issue I'm trying to figure out a good solution for is what our default disassembly/hexview coloring should be. Do we have two color schemes that are "built-in" and pick light/dark like we do with icons? Do we have one (like we do now), and just hope that it looks good by default for most desktops?

Obviously if their theme specifies those things, we're good, they chose it. But what do we do about people who just want to use their system colors?

[10110111]

The only thing I'm still pondering if how the text formatting coloring should interact with this. What I mean is, if we keep the formatting of disassembly and hexviews separate from application theming, they can obviously clash.

It seems pointless to have theming for application and not include text formatting colors into this. I think an EDB theme must include all the relevant colors, including those for disassembly and hex views.

Do we have two color schemes that are "built-in" and pick light/dark like we do with icons?

I think this is the best way. If we have a built-in set of light-on-dark icons, we should also have a builtin default for light-on-dark text.

Obviously if their theme specifies those things, we're good, they chose it. But what do we do about people who just want to use their system colors?

The system colors don't specify syntax highlighting anyway, so we should choose light vs dark builtin by e.g. comparing luminance/brightness/lightness of QPalette::WindowText to that of QPalette::Base. If the latter is darker than the former, most likely we have a dark theme.

[eteran]

Right, we actually do a pretty good job of detecting dark vs light. OK it sounds like your vote is to have any "System" theme also support light/dark, and any non-system theme, well, it'll specify text too (I agree), and possibly have it "inherit" from the built-in light/dark text themes if it is underspecified?

[10110111]

possibly have it "inherit" from the built-in light/dark text themes if it is underspecified?

Yeah, that sounds good.

[eteran]

@10110111 I just pushed a commit to the PR for theming @ https://github.com/eteran/edb-debugger/pull/784 .

I think it implements theming pretty well. There's 3 "built-in" options:

"System" = use system window colors, and auto-choose a default light/dark set of colors for syntax and similar "Dark [built-in" = Forces a fully dark window color scheme (regardless of system settings), and uses a dark mode syntax color scheme "Light [built-in" = Forces a fully light window color scheme (regardless of system settings), and uses a light mode syntax color scheme

Finally...

Any .ini files in a "themes" subdirectory the configuration directory is assumed to be a theme, and will also be in the list of available themes. A user can set as much or as little as they want here. Any unset items will just inherit from "System". So for example, if they want it to look just like edb normally would, except they happen to want addresses to be displayed in blue, they can have a theme file that simply looks like this:

[Theme]
theme.address.foreground=blue

And place it in ~/.config/codef00.com/themes/blue_address.ini

The theme can be selected from the appearance tab of the Options dialog and will take effect after restarting edb.

Let me know what you think!

If you don't find any notable flaws, I'll merge it in and probably call it time to cut the next release :-).

[eteran]

Also, of course, you can view the complete list of themable elements by looking at one of the built-in themes, which are stored in an INI file format identical to user themes:

https://github.com/eteran/edb-debugger/blob/e3491aecf8a4d92063d65ca7dba112cf156a068a/src/themes/dark.ini

[eteran]

Implemented! Time to cut a new release :-D