telegramdesktop / tdesktop

Telegram Desktop messaging app
https://desktop.telegram.org/
Other
26.24k stars 5.21k forks source link

[Feature Request] Notification counter for themed icons on Linux #10407

Closed Shatur closed 1 year ago

Shatur commented 3 years ago

Is your feature request related to a problem?

After version 2.5.5, if the system theme contains a monochrome icon, then the notification counter is not displayed on it.

Before:

изображение

After:

img

This approach may be look nice in some situations, but it is much less practical.

Describe the solution you'd like

It would be nice to add an option that would return the old behavior (displaying notification counter). It would make everyone happy.

As a workaround, users can change the icon theme for Telegram, which does not contain the icon for the system tray. But then the Telegram icon will look different from the rest of the monochrome icons (see how it worked before in the screenshots above).

Additional context

I talked with @ilya-fedin and he said that I should ask @john-preston.

Related issues reported by confused users: #10264, #10251, #10263.

john-preston commented 3 years ago

@Shatur95 It's OK with me if the option is displayed only in that case (it actually changes something).

Shatur commented 3 years ago

@Shatur95 It's OK with me if the option is displayed only in that case (it actually changes something).

~Can be checked using QIcon::hasThemeIcon().~

No, will always return true because the installed app will have an icon in the hicolor folder. But still can be checked by the icon path.

ilya-fedin commented 3 years ago

Can be checked using QIcon::hasThemeIcon().

hasThemeIcon is already used https://github.com/telegramdesktop/tdesktop/blob/dev/Telegram/SourceFiles/platform/linux/main_window_linux.cpp#L155-L159 https://github.com/telegramdesktop/tdesktop/blob/dev/Telegram/SourceFiles/platform/linux/main_window_linux.cpp#L633-L636

No, will always return true because the installed app will have an icon in the hicolor folder.

It works as supposed until you're using distribution package with KDE. KDE injects their bugged libraries and it works wrong. Not a blocker since installation via distribution packages is not recommended anyway.

Shatur commented 3 years ago

hasThemeIcon is already used

I meant that the suggested option might appear when an icon from the theme is used for Telegram (as was suggested by @john-preston). But maybe this option can only exist on Linux without any checks, since the user can change the icons theme at any time?

ilya-fedin commented 3 years ago

I meant that the suggested option might appear when an icon from the theme is used for Telegram

Yeah, I understood. I just mentioned that hasThemeIcon is broken only when KDE injects their bugged libraries. It will work just fine for users of official binary.

Phi-Li commented 3 years ago

I meant that the suggested option might appear when an icon from the theme is used for Telegram

Yeah, I understood. I just mentioned that hasThemeIcon is broken only when KDE injects their bugged libraries. It will work just fine for users of official binary.

Not at all. Just downloaded tsetup.2.8.1.tar.xz from desktop.telegram.org and icon was still the KDE blue dot version. I'm using:

ilya-fedin commented 3 years ago

They added a monochrome icon. Lack of the counter on monochrome is expected, monochrome icons is exactly the reason this check was added. If you want the counter, you should use icon pack without monochrome icon.

Shatur commented 3 years ago

If you want the counter, you should use icon pack without monochrome icon.

Yes, and this proposal suggests to apply the counter even on monochrome icons, as it was before. Because at the moment the user can have either a monochrome icon that matches his theme, but without a counter, or a counter, but with an icon that stands out in the system tray. The developers seems to agree, but I need to find time to send a PR :)

ilya-fedin commented 3 years ago

Yes, and this proposal suggests to apply the counter even on monochrome icons, as it was before.

No, thanks, this will break color schemes again :)

Shatur commented 3 years ago

No, thanks, this will break color schemes again :)

This should be a colorscheme issue if it defines a wrong icon. Also the user can even fix it yourself by simply replacing the icon. But we decided to just add it as an option. And @john-preston agreed with my proposal as far as I understood.

ilya-fedin commented 3 years ago

This should be a colorscheme issue if it defines a wrong icon.

The thing is, color scheme is applied on panel side. When the counter is being added, the icon is rasterized and sent as a bitmap, so applying color scheme is no longer possible.

Also the user can even fix it yourself by simply replacing the icon

You can do the same to get counter

ilya-fedin commented 3 years ago

And @john-preston agreed with my proposal as far as I understood.

He agreed that there could be an option, not a revert (you said "as it was before", so I assume you meant revert)

Shatur commented 3 years ago

The thing is, color scheme is applied on panel side. When the counter is being added, the icon is rasterized and sent as a bitmap, so applying color scheme is no longer possible.

I thought we talking about icon themes. How color schemes can be broken by the old behavior?

You can do the same to get counter

Yes, technically I can replace cached icons with monochrome by modifying every cached icon with counter, but let's be realistic :)

He agreed that there could be an option, not a revert (you said "as it was before", so I assume you meant revert)

Yes, sorry for by bad English, I meant to add a parameter that restores the previous behavior.

ilya-fedin commented 3 years ago

How color schemes can be broken by the old behavior?

KDE applies color scheme to svg icons. You can change the color of monochrome icons by chaning color scheme in KDE settings. When icons are rasterized, this is no longer possible and here angry users come to issues (#10239).

Yes, technically I can replace cached icons with monochrome by modifying every cached icon with counter, but let's be realistic :)

WTF, what cache are you referring to? tdesktop doesn't cache tray icon starting from 1.9.x. If you want to get the counter, all you need is to remove telegram-{,attention-,mute-}panel.png from your icon theme.

Shatur commented 3 years ago

KDE applies color scheme to svg icons. You can change the color of monochrome icons by chaning color scheme in KDE settings.

Oh, got it. Never used this feature before. Thanks for the explanation. Yes, this is definitely should be an option.

WTF, what cache are you referring to? tdesktop doesn't cache tray icon starting from 1.9.x.

Then it was like this before. In this case, it is impossible to get a monochrome icon with an indicator counter at all.

If you want to get the counter, all you need is to remove telegram-{,attention-,mute-}panel.png from your icon theme.

Yes, I know, and I will have the default icon instead of изображение. This is what I'm talking about :) Maybe I will try to reintroduce this feature as an option and send a PR.

ilya-fedin commented 3 years ago

Yes, I know, and I will have the default icon instead of изображение.

You can rename telegram-panel.png to telegram.png and remove the two others (telegram-{attention,mute}-panel.png)

ilya-fedin commented 3 years ago

Maybe I will try to reintroduce this feature as an option and send a PR.

It's something weird to add the counter when icon already has a dot indicator, btw.

ilya-fedin commented 3 years ago

I would say it's more correct to make icons with counter for icon theme creators rather than creating an option that would restore this weird behavior.

Shatur commented 3 years ago

You can rename telegram-panel.png to telegram.png and remove the two others (telegram-{attention,mute}-panel.png)

This is how it looks after the suggested changes:

изображение

It's something weird to add the counter when icon already has a dot indicator, btw.

But dot indicator is useless :) The notification counter was much better. But it would be nice to have it on monochrome icons. And I'm not the only one, judging by the issues. Probably #10239 is the only issue, while we have many other issues about the current behavior. So it looks like we removed a good feature only because of the weird KDE behavior.

I would say it's more correct to make icons with counter for icon theme creators rather than creating an option that would restore this weird behavior.

I fully understand your position on adding this option. But how that can be achieved on the icon themes side?

ilya-fedin commented 3 years ago

This is how it looks after the suggested changes:

Are you sure you did it right, so that telegram.png contains the monochrome icon and none of telegram-{,attention-,mute-}panel.png exist?

But dot indicator is useless :) The notification counter was much better.

None of icon theme creators had counter support on the time of adding icon theme support AFAIK.

I fully understand your position on adding this option. But how that can be achieved on the icon themes side?

tdesktop can ask icons with count value in its name, e.g. telegram-{attention,mute}-panel-{1-999}.png and telegram-{attention,mute}-panel-dotdot{1-99}.png

Shatur commented 3 years ago

Are you sure you did it right, so that telegram.png contains the monochrome icon and none of telegram-{,attention-,mute-}panel.png exist?

Yes. I use papirus icon theme. But the icons are in .svg format, is this important?

find . -name 'telegram.*'
./24x24/apps/telegram.svg
./24x24/panel/telegram.svg # <- Monochrome, was renamed from `telegram-panel`
./22x22/apps/telegram.svg
./22x22/panel/telegram.svg # <- Monochrome, was renamed from `telegram-panel`
./32x32/apps/telegram.svg
./64x64/apps/telegram.svg
./16x16/apps/telegram.svg
./16x16/panel/telegram.svg # <- Monochrome, was renamed from `telegram-panel`

None of icon theme creators had counter support on the time of adding icon theme support AFAIK. tdesktop can ask icons with count value in its name, e.g. telegram-{attention,mute}-panel-{1-999}.png and telegram-{attention,mute}-panel-dotdot{1-99}.png

But would that require adding 999 new icons to the theme?

Phi-Li commented 3 years ago

OK, time to bring some temporary workaround for this issue. For Archlinux users you need to rename/remove all Telegram icon files from package breeze-icons:

/usr/share/icons/breeze/apps/48/telegram.svg
/usr/share/icons/breeze/status/22/telegram-panel.svg
/usr/share/icons/breeze/status/22/telegram-attention-panel.svg
/usr/share/icons/breeze/status/22/telegram-mute-panel.svg
/usr/share/icons/breeze/status/24/telegram-panel.svg
/usr/share/icons/breeze/status/24/telegram-attention-panel.svg
/usr/share/icons/breeze/status/24/telegram-mute-panel.svg
/usr/share/icons/breeze-dark/apps/48/telegram.svg
/usr/share/icons/breeze-dark/status/22/telegram-panel.svg
/usr/share/icons/breeze-dark/status/22/telegram-attention-panel.svg
/usr/share/icons/breeze-dark/status/22/telegram-mute-panel.svg
/usr/share/icons/breeze-dark/status/24/telegram-panel.svg
/usr/share/icons/breeze-dark/status/24/telegram-attention-panel.svg
/usr/share/icons/breeze-dark/status/24/telegram-mute-panel.svg

Now I have the original tray icon for Archlinux-packed Telegram. As a long-term user since early versions of Plasma, I'm used to modify theme files to revert their exciting changes in a new version.

ilya-fedin commented 3 years ago

Yes. I use papirus icon theme. But the icons are in .svg format, is this important?

I guess KDE doesn't prefer panel over apps ones, lol. I think you need to move them to apps folder and remove original ones in other sizes. But that will lead to app icon being monochrome...

But would that require adding 999 new icons to the theme?

Yeah, unfortunately. Another solution would be to add tray protocol extension to set the count, so that the counter is drawn by panel. But I don't know who I need to talk to submit one.

Shatur commented 3 years ago

I guess KDE doesn't prefer panel over apps ones, lol. I think you need to move them to apps folder and remove original ones in other sizes. But that will lead to app icon being monochrome...

Wow, this is weird :) But can be a temporary solution, thanks!

Yeah, unfortunately.

Hm... The issue in rasterization, right? What if we check the format of the icon and if it's SVG then modify it in place without a rasterization (just by adding text) to add an indicator counter?

ilya-fedin commented 3 years ago

What if we check the format of the icon and if it's SVG then modify it in place without a rasterization (just by adding text) to add an indicator counter?

Someone should write the code to modify svg icons. I definitely won't be the one who will do this and I don't think @john-preston will do this. If you will work on this, then this has a chance to happen :)

ilya-fedin commented 3 years ago

It's a question though, how panel implementations will react to this. The API allows to pass raster bitmaps or icon name in icon theme, it doesn't allow to pass svg content. You can try to save svg icon to /tmp and pass the path in IconName field (that 100% works for png, this is used as a workaround for Ubuntu implementations (they replace most DE's tray implementations with their custom one) of the protocol that don't support IconPixmap), but I'm not sure will this work or not. If not, this would require creating an icon theme on runtime (e.g. creating a folder in ~/.local/share/icons, creating a index.theme file, creating subfolders, icon files, etc.).

ilya-fedin commented 3 years ago

Now I have the original tray icon for Archlinux-packed Telegram.

tdesktop sets setDesktopSettingsAware(false) currently (for multiple last versions) as a workaround for qt plugin system/webkit2gtk conflict, but once gtk remoting will arrive at stable version, this workaround will be removed and tdesktop from distro builds will use KDE's platformtheme again and you will be affected by the KDE bug mentioned in the thread, so you will lose the counter.

Phi-Li commented 3 years ago

tdesktop sets setDesktopSettingsAware(false) currently (for multiple last versions) as a workaround for qt plugin system/webkit2gtk conflict, but once gtk remoting will arrive at stable version, this workaround will be removed and tdesktop from distro builds will use KDE's platformtheme again and you will be affected by the KDE bug mentioned in the thread, so you will lose the counter.

Not an expert in GUI dev. So in future KDE won't fallback to icon files Telegram provides even if themed icon files are removed? Or current implementation of counter on the tray icon won't work in future?

ilya-fedin commented 3 years ago

Not an expert in GUI dev. So in future KDE won't fallback to icon files Telegram provides even if themed icon files are removed? Or current implementation of counter on the tray icon won't work in future?

Qt has a plugin system that allows DEs to inject their libraries to integrate applications. As was mentioned here, KDE libraries have a bug that leads the check to always return true, so that the counter will be never added to the icon. Multiple latest versions have a workaround that disables that system to avoid gtk have different backend (wayland/x11) and make window embedding work (for the bot payment feature). Once the workaround won't be needed, KDE will be able to inject libraries again and the KDE bug will affect tdesktop again when running on KDE.

If you will use official binary, you won't be affected since it can't load system libraries (since they're linked to a different Qt version).

Shatur commented 3 years ago

@ilya-fedin Yes, I would try ti implement, but probably it's a hacky solution... I think that the our first suggestion to add an option "Draw counter on themed icon" will be a better solution. Will you accept my PR if I implement it?

Once the workaround won't be needed, KDE will be able to inject libraries again and the KDE bug will affect tdesktop again when running on KDE.

I do not know how it was before, but QIcon::hasThemeIcon works 100% correctly in my KDE environment. Maybe the issue was specific to KDE Neon or just was fixed by developers. So, there is nothing to worry about, @Phi-Li :) In my message https://github.com/telegramdesktop/tdesktop/issues/10407#issuecomment-782694342 I meant that QIcon::hasThemeIcon("telegram") will always return true for my case because this icon is installed by the package. Sorry If I confuse you.

ilya-fedin commented 3 years ago

Will you accept my PR if I implement it?

It's a question for @john-preston, I have no push access.

I do not know how it was before, but QIcon::hasThemeIcon works 100% correctly in my KDE environment.

Try to invoke QIcon::hasThemeIcon("telegram-panel") when telegram.svg is present, but telegram-panel.svg is not.

Shatur commented 3 years ago

Try to invoke QIcon::hasThemeIcon("telegram-panel") when telegram.svg is present, but telegram-panel.svg is not.

Returns false for me. Is this expected?

ilya-fedin commented 3 years ago

Returns false for me. Is this expected?

Are you sure KDE injects its libraries? It returns true for me on KDE in that case and false on all other DEs.

Shatur commented 3 years ago

Are you sure KDE injects its libraries? It returns true for me on KDE in that case and false on all other DEs.

How can I check this? Do you have an older version of Qt / KDE? If you are using KDE Neon, this may be a distro issue. I use Qt5 5.15.2 and Plasma 5.22.3.

ilya-fedin commented 3 years ago

How can I check this?

:thinking: with QT_DEBUG_PLUGINS=1

Do you have an older version of Qt / KDE? If you are using KDE Neon, this may be a distro issue. I use Qt5 5.15.2 and Plasma 5.22.3.

I see the bugged place is still here: https://github.com/KDE/kiconthemes/blob/783296891ba07bc5a78a498b9425571b391d21be/src/kiconengine.cpp#L110-L116

QIcon::hasThemeIcon constructs a QIcon and checks that its name is equal to what you requested, KIconEngine always returns the name that was requested instead of actual name (Qt's built-in icon engine returns actual name).

Shatur commented 3 years ago

thinking with QT_DEBUG_PLUGINS=1

Here is my log: log.txt

I see the bugged place is still here:

So mIconLoader->hasIcon(mIconName) always returns true for you?

ilya-fedin commented 3 years ago

So mIconLoader->hasIcon(mIconName) always returns true for you?

This has nothing to do with mIconLoader->hasIcon(mIconName), it's return mIconName;

QIcon::hasThemeIcon constructs a QIcon and checks that its name is equal to what you requested

ilya-fedin commented 3 years ago

Here is my log: log.txt

either your distro patches the library or you don't have telegram.svg

Shatur commented 3 years ago

either your distro patches the library or you don't have telegram.svg

Oh, sorry, I just called it before QApplication. Yes, QIcon::hasThemeIcon("telegram-panel") returns true because it fallbacks to telegram. And when I construct the icon it uses telegram icon but says that the name is telegram-panel. How you expect it to work?

ilya-fedin commented 3 years ago

How you expect it to work?

QIcon::hasThemeIcon should return false in that case and QIcon::name should return actual name, just like pure Qt does

Shatur commented 3 years ago

QIcon::hasThemeIcon should return false in that case and QIcon::name should return actual name, just like pure Qt does

Got it, thanks. Even found a related bug report.

Shelvak commented 3 years ago

That's happening with a new gnome install with manjaro/arch.

telegram-desktop-3.1.0-1 (manjaro) telegram-desktop-3.1.1-1 (Arch) Gnome 40.4.0

Any workaround? :pray:

EDIT: I had a lot of svg's from papirus (I guest that is because of manjaro-gnome) so I deleted them all and get the old icon with counter working :D

soredake commented 2 years ago

https://bugs.kde.org/show_bug.cgi?id=432293

soredake commented 2 years ago

Is this really "completed"?

ilya-fedin commented 2 years ago

I don't know why @Aokromes closed the issue, but there's no plan to implement this feature, sorry. We consider the current UX where the counter is displayed only in the taskbar icon good enough.

soredake commented 2 years ago

I'm kinda fine with notification counter in taskbar icon, but in kde, unlike in windows, counter does not have red background, will need to report this to kde, not sure which component is responsible for counter background color.

ilya-fedin commented 2 years ago

On Windows it's painted by tdesktop itself and is set with WinAPI as icon overlay, on Linux there's no such API, but there's API to send the counter number and then the panel draws it. I guess it should follow your color scheme in KDE.

Shatur commented 2 years ago

but there's no plan to implement this feature, sorry.

That's bad. It was much better before when we had a counter on icon.

ilya-fedin commented 2 years ago

Point raised by @john-preston in https://github.com/telegramdesktop/tdesktop/issues/10407#issuecomment-782693898 should still be valid, so it just needs someone interested to implement that (definitely not me as I like the current behavior and apparently not @john-preston).

github-actions[bot] commented 1 year ago

Hey there!

This issue was inactive for a long time and will be automatically closed in 30 days if there isn't any further activity. We therefore assume that the user has lost interest or resolved the problem on their own.

Don't worry though; if this is an error, let us know with a comment and we'll be happy to reopen the issue.

Thanks!