[breeze-icons] Add telegram-desktop tray icons
ClosedPublic

Authored by rocka on Feb 24 2020, 9:35 AM.

Details

Summary

Recent version of telegram-desktop added support for using system icon theme for tray icon. This patch adds some icons following breeze style.

According to comments on GitHub , pass environemnt variable TDESKTOP_DISABLE_TRAY_COUNTER=1 to telegram-desktop also make it follows KDE color scheme.

BUG:417583

Test Plan

Before:

After:

breezebreeze-dark

Update:

breezebreeze-dark

Diff Detail

Repository
R266 Breeze Icons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rocka created this revision.Feb 24 2020, 9:35 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 24 2020, 9:35 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
rocka requested review of this revision.Feb 24 2020, 9:35 AM
davidre edited the summary of this revision. (Show Details)Feb 24 2020, 9:49 AM
davidre added a reviewer: Fuchs.

Thanks for the patch!

But...it's blue: :)

ndavis added a subscriber: ndavis.Feb 24 2020, 2:40 PM

Thanks for the patch!

But...it's blue: :)

Weird. I can't see anything obviously wrong in the code. I'll test it later.

rocka added a comment.Feb 24 2020, 2:45 PM

What's the version of telegram-desktop? Would it still be blue if change workspace theme to breeze-dark? If it works fine in breeze-dark and telegram version is 1.9.12, it maybe a upstream bug , and was fixed in the latest 1.9.14 release.

Yes, I have 1.9.12. I don't yet have an available update to 1.19.14 or later, sadly.

rocka added a comment.Mon, Mar 2, 3:18 PM

Are we ready for some reviews?

ndavis added a comment.Mon, Mar 2, 4:39 PM

Sorry, I've been busy lately.

I noticed some issues:

  1. The distinguishing part of the icon doesn't take up much space, so too much of it gets hidden behind the notification badge.
    • I'd suggest removing the circle and making the distinguishing part larger.
  2. The style should match the official logo better. I'm removing the 48px breeze icons in D27787 since they're outdated and the official icon is extremely close to being a Breeze icon.
    • You could just copy the relevant bits from Papirus icons and resize/edit them for Breeze icons.
  3. Since I'm removing the 48px Breeze icons, you'll have to make a patch to the desktoptheme in plasma-frameworks instead of Breeze icons or else breeze will use monochrome icons for telegram everywhere. Sorry for the inconvenience, but feel free to message me on Matrix (@noahdvs:kde.org) or Telegram (@noahdvs) if you need help.
rocka updated this revision to Diff 76942.Wed, Mar 4, 2:15 PM
rocka edited the test plan for this revision. (Show Details)

I've re-make the distinguishing part of the icons following the official one, it would be easy to recognize now. See the "Update" part in "Test Plan".

Since the name for tray icons are telegram-{,attention-,mute-}panel, it would not affect the application icon elsewhere, whose name is telegram-desktop.

rocka updated this revision to Diff 76946.Wed, Mar 4, 3:28 PM

Fix attention dot color and remove accidentally added patch file

ndavis accepted this revision.Thu, Mar 5, 3:14 AM

You're pretty good at this. Most newbies struggle with all the little things that can bite you when making breeze icons.

This revision is now accepted and ready to land.Thu, Mar 5, 3:14 AM
ndavis updated this revision to Diff 76987.Thu, Mar 5, 3:39 AM
ndavis edited the test plan for this revision. (Show Details)

Update author info

This revision was automatically updated to reflect the committed changes.

This breaks for people using Breeze for apps and Breeze Dark for Plasma:

pass environemnt variable TDESKTOP_DISABLE_TRAY_COUNTER=1 to telegram-desktop also make it follows KDE color scheme

This is not a solution; we need to make sure things work by default and not in terminal-hackish way. So for a novice user with default Kubuntu settings and an official Telegram build, "the icon is broken in KDE" now.
Tbh I'd rather we didn't interfere with 3rd-party apps at all.

ndavis added a comment.EditedTue, Mar 17, 8:28 PM

This breaks for people using Breeze for apps and Breeze Dark for Plasma:

pass environemnt variable TDESKTOP_DISABLE_TRAY_COUNTER=1 to telegram-desktop also make it follows KDE color scheme

This is not a solution; we need to make sure things work by default and not in terminal-hackish way. So for a novice user with default Kubuntu settings and an official Telegram build, "the icon is broken in KDE" now.
Tbh I'd rather we didn't interfere with 3rd-party apps at all.

Why does it need that environment variable? This seems to be a problem with Telegram, so maybe it should be fixed in Telegram?

Unfortunately, it looks like I'll have to revert this patch because when people use a light colorscheme with a dark plasma theme, the icons turn black because the icon is rendered as a pixmap when it has the red counter badge.

Without badge... Same problem? I'm using dark plasma theme (but not Breeze-dark specifically, it is Sweet KDE) and normal Breeze for apps

Would it help this use case if we moved the Telegram icon into the Plasma theme for now, pending a fix in Telegram itself? On that subject, has anyone submitted a bug report to them about their inappropriate use of pixmaps?

ndavis added a subscriber: broulik.EditedWed, Mar 18, 7:17 PM

Would it help this use case if we moved the Telegram icon into the Plasma theme for now, pending a fix in Telegram itself? On that subject, has anyone submitted a bug report to them about their inappropriate use of pixmaps?

@broulik said that moving to the plasma theme wouldn't work. In the conversation yesterday, it sounded like it wasn't necessarily wrong to use pixmaps.

So... how do we fix this so that you can use a nice monochrome Breeze icon in your system tray? Whose code needs to change?

So... how do we fix this so that you can use a nice monochrome Breeze icon in your system tray? Whose code needs to change?

  1. SNI implementation in Plasma needs to be fixed to properly support OverlayIconPixmap with IconName. From what I understand, since it affects a data engine and that is considered public API, this might have to wait for Plasma 6
  2. Since old versions of Plasma will still have the issue, and other SNI implementations likely don't support this either, this can't become the default behavior for Telegram; so there has to be a way for Telegram to check if an SNI implementation supports what it needs
  3. Then Telegram code needs to add a special case for that.

Since I feel like a proxy at this point, tagging @davidre and @ilya-fedin.

So... how do we fix this so that you can use a nice monochrome Breeze icon in your system tray? Whose code needs to change?

  1. SNI implementation in Plasma needs to be fixed to properly support OverlayIconPixmap with IconName. From what I understand, since it affects a data engine and that is considered public API, this might have to wait for Plasma 6
  2. Since old versions of Plasma will still have the issue, and other SNI implementations likely don't support this either, this can't become the default behavior for Telegram; so there has to be a way for Telegram to check if an SNI implementation supports what it needs
  3. Then Telegram code needs to add a special case for that.

    Since I feel like a proxy at this point, tagging @davidre and @ilya-fedin.

Huh that case is working what I found that IconName and OverlayIconName is currently broken, see D28107.
IconName and OverlayIconPixmap` work fine here

If it doesn't for you please file a bug so we can investigate that.

Huh that case is working what I found that IconName and OverlayIconName is currently broken, see D28107.
IconName and OverlayIconPixmap` work fine here

If it doesn't for you please file a bug so we can investigate that.

Btw that overlay is always as tiny as on your screenshot, so not usable for a counter

Would it help this use case if we moved the Telegram icon into the Plasma theme for now, pending a fix in Telegram itself?

There is nothing to do on Telegram side. Everything that Telegram can get is icon theme name (sometime), does the icon exist, the icon itself and its sizes. Adding a dependency on kde frameworks is not a way, of course.

Btw that overlay is always as tiny as on your screenshot, so not usable for a counter

+1. Moreover, icon is not recolored even with IconName + OverlayIconPixmap.

Would it help this use case if we moved the Telegram icon into the Plasma theme for now, pending a fix in Telegram itself?

There is nothing to do on Telegram side. Everything that Telegram can get is icon theme name (sometime), does the icon exist, the icon itself and its sizes. Adding a dependency on kde frameworks is not a way, of course.

Btw that overlay is always as tiny as on your screenshot, so not usable for a counter

+1. Moreover, icon is not recolored even with IconName + OverlayIconPixmap.

Oh yes that's true :/. It's a similiar bug to the one I fixed not long ago. It seems the SNI desperately needs the change as outlined in the other diff by removing all the rendering from the data engine. This way also the sizing of the overlay, I think I noticed it being inconsistent between pixmap and overlayname path.