search

Martchus / syncthingtray

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

Use icons from KDE theme #121

Closed mrpink17 closed 1 year ago

mrpink17 commented 2 years ago

Relevant components

Is your feature request specific to a certain platform/environment? Please specify. KDE Plasma

Is your feature request related to a problem? Please describe. The app uses custom icons that don't look "standard" in KDE Plasma, they are also a bit blurry

Describe the solution you'd like Use icons from KDE theme

Additional context Screenshot_20211215_231815

Martchus commented 2 years ago

I'm using ForkAwesome icons in consistency with Syncthing's official GUI. Especially for the icons within the expandable list items that makes also most sense in my opinion. For consistency with that I also like to use ForkAwesome for the icons close to that list view. That has also the advantage (over the system icon theme) that I can rely on the icons being actually present. The ForkAwesome icons are quite neutral themselves so they should fit in most icon themes (and follow your color settings which has been recently improved by https://github.com/Martchus/syncthingtray/commit/3e38a9917e497a77fa270876535fca84d940a4ff).

Maybe it could be made configurable, e.g. adding an option to use the system's icon theme as much as possible. Note that I personally prefer the ForkAwesome icons so I'm not likely implementing this soon (as it is likely quite some effort).

mrpink17 commented 2 years ago

Yes, it would be great!

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.

sten0 commented 2 years ago

Because Syncthing is a "killer app", I think that in the future the KDE project may be amenable to a PR that adds the required SVG icons to the base set. This would take care of question of icon availability. If a new enough version of Plasma is not detected at compile time, then qtforkawesome would be the only available option. It also seems like Qtforkawesome must be retained for use of the tray icon on non KDE Plasma systems.

I worry that mixing the two might cause scaling issues on HiDPI displays that use a non-default scaling factor. More large-scale use will reveal if users want sharper icons, and HiDPI users can be quite passionate about this sort of thing.

No comment on aesthetics, and I believe there's an argument to be made about how using the same icons as the Syncthing web interface makes the following semantic statement: it makes the user aware they're interacting with a web thing rather than a desktop thing. That can be a good or a bad thing, depending on one's perspective.

Martchus commented 2 years ago

Note that currently my approach is to use icons from the normal icon theme everywhere (dialogs, context menus, …) except for the main tray dialog where I use ForkAwesome icons. Especially for the view it just makes more sense to be consistent with Syncthing's normal GUI. Note that ForkAwesome is quite neutral and look good together with most other icon themes (and Plasma themes) and the icons will follow the system's color palette. HiDPI should not be a concern because the font icons also scale well.

However, I'm also open for implementing a config switch to use system icons as aggressively as possible. I just haven't implemented it myself because I personally don't see it as better.

sten0 commented 2 years ago

Note that currently my approach is to use icons from the normal icon theme everywhere (dialogs, context menus, …) except for the main tray icon where I use ForkAwesome icons. Especially for the view it just makes more sense to be consistent with Syncthing's normal GUI. Note that ForkAwesome is quite neutral and look good together with most other icon themes (and Plasma themes) and the icons will follow the system's color palette. HiDPI should not be a concern because the font icons also scale well.

However, I'm also open for implementing a config switch to use system icons as aggressively as possible. I just haven't implemented it myself because I personally don't see it as better.

:-) I agree that it's a question of preference, and choice of orientation. At this time I don't have a strong preference either way (although I'm biased towards svg, and am also a supporter of user theming). I agree that HiDPI shouldn't be a problem, but the icons in @mrpink17's screenshot do look a bit blurry to me, but to be fair, so do the fonts. It looks like it might be Ubuntu-style hinting and/or aliasing. Does Qt ForkAwesome already override desktop preferences like these? If not, that might be a way to resolve the fuzziness. In theory, font icons should scale similarly well to svg ones after all!

Martchus commented 2 years ago

Does Qt ForkAwesome already override desktop preferences like these?

It renders the glyphs like any other text would be rendered within the Qt application. Normally Qt is using freetype2 to render fonts (at least under GNU/Linux) so settings affecting freetype2 should have effect. The settings done via systemsettings5 definitely have effect (although you need to restart the application, in case of the Plasmoid that's plasmashell itself).

And yes, the screenshot looks blurry (including the normal text rendering).

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.

sten0 commented 2 years ago

Martchus @.***> writes:

Does Qt ForkAwesome already override desktop preferences like these?

It renders the glyphs like any other text would be rendered within the Qt application. Normally Qt is using freetype2 to render fonts (at least under GNU/Linux) so settings affecting freetype2 should have effect. The settings done via systemsettings5 definitely have effect (although you need to restart the application, in case of the Plasmoid that's plasmashell itself).

Yes, and it is possible to apply font-specific settings in cases where the user-specific desktop settings are insufficient; for example, some fonts are designed to be used with hinting, and others are designed to be used without it; the fonts.conf method allows both to look good on the same system.

https://www.freedesktop.org/software/fontconfig/fontconfig-user.html https://wiki.archlinux.org/title/font_configuration

It should be possible make Qt ForkAwesome inject these settings. For example, to disable antialiasing and sub-pixel rendering to produce sharper results. This would mitigate the effects of a distribution's freetype2 patches, if they result in poor rending of font icons.

Were Syncthing Tray to use Fork Awesome directly, I'd say Fork Awesome could/maybe should provide a /etc/fonts/font.d/??fork-awesome.conf, but I feel like this method may not work with the Qt ForkAwesome. On the upside, it seems like the use of Qt ForkAwesome provides the opportunity to fix Qt-ForkAwesome-using applications on systems that won't receive an updated fonts-forkawesome package (eg: all existing Ubuntu releases, and their derivatives) which allows them to be fixed.

And yes, the screenshot looks blurry (including the normal text rendering).

Fiouf, it's not just my eyes ;) I think this is probably evidence of Ubuntu's freetype2 patches, which privilege "smoothness" above "sharpness" (like early 2000s era MacOS-style font rendering). People who like this style argue that the shapes are more true; this group really doesn't like "jagged" fonts. People who prefer sharpness don't can't get past the blurriness.

To be fair, this is a cosmetic issue...

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.

sten0 commented 2 years ago

ping

Martchus commented 2 years ago

Adding the enhancement label so stale bot won't close it.

Note that the main problem is that the free desktop specification doesn't provide all icons we need so we'd still end up with a mix of KDE and ForkAwesome icons. Therefore I don't think that this is very worthwhile. At least one needs to ensure that the mixup won't be too bad (e.g. still use ForkAwesome consistently in the expandable details). Of course it is actually already a mixup but at least with a clear distinction where which icons are used (see https://github.com/Martchus/syncthingtray/issues/121#issuecomment-1088473032).

Martchus commented 2 years ago

Implemented for the Qt Widgets version and the Plasmoid as optional feature. On the Plasmoid it looks rather ugly when the current freedesktop icon theme (e.g. Breeze light) doesn't match the Plasma theme (e.g. some dark theme). I suppose I actually needed to use the icons from the Plasma theme here (and not from the freedesktop icon theme).

Martchus commented 1 year ago

I suppose I actually needed to use the icons from the Plasma theme here (and not from the freedesktop icon theme).

It looks like this aspect is not going to be relevant in Plasma 6 anymore (see https://pointieststick.com/2023/07/26/what-we-plan-to-remove-in-plasma-6). Since it was the only improvement left to do I'm closing this issue.