Martchus / syncthingtray

Tray application and Dolphin/Plasma integration for Syncthing
https://martchus.github.io/syncthingtray/
Other
1.68k stars 44 forks source link

plasmoid uses icons which are dark in plasma dark theme #126

Closed lbckmnn closed 2 years ago

lbckmnn commented 2 years ago

Relevant components

Environment and versions

Bug description The icons of the Plasmoid are barely visible on breeze-dark plasma theme. The screenshot in the repository description shows a plasmoid with light icons. I guess the plasmoid just uses the wrong ones?

I also made sure that the Breeze Dark icon set is selected in system settings. Is there an option to choose the icon set?

Steps to reproduce

  1. set plasma theme to breeze-dark
  2. install syncthingtray and place plasmoid

Expected behavior plasmoid uses the appropriate icons for plasma theme.

Screenshots Screenshot_20220201_203816

Martchus commented 2 years ago

Light colors for custom color roles (roles not provided by QPalette and hence not deducible from KDE's color settings) can be enabled in the settings. It isn't nice that this setting cannot be determined automatically but so far I refrained from adding logic which would guess it (from common color roles).

lbckmnn commented 2 years ago

where exactly do i find this settings? I would be also willing to help but i have no experience with the syncthingtray code base.

Martchus commented 2 years ago

Plasmoid/Tray -> Apearance -> Colors -> Bright custom text colors (use for dark color scheme)

By the way, the icon theme has no effect here because these are technically text colors.

Martchus commented 2 years ago

Oh, wait - the color of the ForkAwesome icons should actually follow the normal foreground text color from KDE's color settings. Could it be that you have a light color theme in KDE's general color selection but nevertheless using a dark Plasma theme? I suppose in this case the coloring can actually be mixed up.

lbckmnn commented 2 years ago

Yes the "Bright custom text colors" has no effekt.

Ah yes, is use the "Breeze Twighlight" global theme. This seems to use the breeze light theme for "Colors" but "Breeze Dark" for Plasma Style

Martchus commented 2 years ago

Well, not sure whether that's the case in your setup but you definitely uncovered a bug that should be fixed. Now the question is how to read colors from the Plasma theme (rather than QPalette).

lbckmnn commented 2 years ago

Choosing "Breeze Dark" does indeed result in white icons. Other plasmoids (e.g. the blueetooth plasmoid) are using white icons when Plasma uses Breeze Dark but colors are set to Breeze (light). Maybe we could look there?

Martchus commented 2 years ago

Not really. I'm not actually rendering icons here, I'm rendering text (as the ForkAwesome icons are actually just font glyphs). All this is done in C++ and I cannot easily use the usual Qt Quick components for displaying them.

lbckmnn commented 2 years ago

Maybe a checkbox for dark/light color similar to the "Bright custom text colors" could be used as workaround?

Martchus commented 2 years ago

Looks like the answer is: https://api.kde.org/frameworks/plasma-framework/html/classPlasma_1_1QuickTheme.html

Now I only need to find a way to access it from C++.

lbckmnn commented 2 years ago

It is possible to just instantiate a Plasma::Theme with the default constructor. That instance will then be the currently used plasma theme. The text color can then be accessed easily. No need to use Plasma::QuickTheme.

See the minimal example:


#include "mainwindow.h"
#include <QApplication>
#include <iostream>
#include <Plasma/Theme>

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    MainWindow w;
    Plasma::Theme currentPlasmaTheme(&w);
    std::cout << "current theme name: " << currentPlasmaTheme.themeName().toStdString() << std::endl;
    std::cout << currentPlasmaTheme.color(Plasma::Theme::TextColor).name().toStdString() << std::endl;
    w.show();
    return a.exec();
}

When using Plasma theme Breeze Dark the output is:

current theme name: breeze-dark
#fcfcfc

When using Breeze, the output is:

current theme name: default
#232627

I am willing to implement this in syncthingtray but i would need a couple of days because it has been a while since I last used Qt.

Martchus commented 2 years ago

I've just gave it a quick try as well and this works, indeed. Somehow my QtForkAwesome::QuickImageProvider still doesn't respect the color but I suppose I'll be able to sort it out.

Martchus commented 2 years ago

It should be fixed on master. (The problem why QtForkAwesome::QuickImageProvider didn't work was that I had the Git version of Syncthing Tray installed on my system and plasmashell's QML engine was using the image provider from there. Took me a while to figure that out, the coding was the easy part.)

Martchus commented 2 years ago

While https://github.com/Martchus/syncthingtray/commit/3e38a9917e497a77fa270876535fca84d940a4ff fixes the issue there's still one problem left: Theme changes aren't immediately applied (only after restarting plasmashell). I tried fixing this as well (see https://github.com/Martchus/syncthingtray/commit/9b7d6263e8e04a523229f302e8ee66e2f696f70b) but haven't had any luck so far. I'm not sure how to tell the Qt Quick Image to reload the image. Changing the source back and forth doesn't have an effect, even when caching is disabled.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Martchus commented 2 years ago

I'd actually still like to fix the problem mentioned in my previous comment. I just don't know how it is done in Qt Quick.

Martchus commented 2 years ago

The problem mentioned in https://github.com/Martchus/syncthingtray/issues/126#issuecomment-1032523273= should be fixed by 8ec133e62d757ae81dac53212053e202fb4f0a89.