D27617: [breeze-icons] Add telegram-desktop tray icons

[David Redondo]

davidre added a comment.


  In D27617#630440 , @ilya-fedin 
wrote:
  
  > In D27617#629987 , @ngraham 
wrote:
  >
  > > 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.
  >
  > In D27617#630392 , @IlyaBizyaev 
wrote:
  >
  > > 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.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: davidre, ilya-fedin, broulik, alexeymin, IlyaBizyaev, ndavis, ngraham, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Ilya Fedin]

ilya-fedin added a comment.


  In D27617#629987 , @ngraham wrote:
  
  > 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.
  
  In D27617#630392 , @IlyaBizyaev 
wrote:
  
  > 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.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: davidre, ilya-fedin, broulik, alexeymin, IlyaBizyaev, ndavis, ngraham, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Ilya Bizyaev]

IlyaBizyaev added a comment.


  In D27617#630339 , @davidre wrote:
  
  > Huh that case is working what I found that `IconName` and `OverlayIconName` 
is currently broken, see D28107 .
  >  `IconName and `OverlayIconPixmap` work fine  here F8184158: 
Screenshot_20200319_102827.PNG 
  >
  > 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

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: davidre, ilya-fedin, broulik, alexeymin, IlyaBizyaev, ndavis, ngraham, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[David Redondo]

davidre added a comment.


  In D27617#630336 , @IlyaBizyaev 
wrote:
  
  > In D27617#630228 , @ngraham 
wrote:
  >
  > > 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 F8184158: 
Screenshot_20200319_102827.PNG 
  
  If it doesn't for you please file a bug so we can investigate that.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: davidre, ilya-fedin, broulik, alexeymin, IlyaBizyaev, ndavis, ngraham, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Ilya Bizyaev]

IlyaBizyaev added subscribers: ilya-fedin, davidre.
IlyaBizyaev added a comment.


  In D27617#630228 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: davidre, ilya-fedin, broulik, alexeymin, IlyaBizyaev, ndavis, ngraham, 
kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: broulik, alexeymin, IlyaBizyaev, ndavis, ngraham, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Noah Davis]

ndavis added a subscriber: broulik.
ndavis added a comment.


  In D27617#629987 , @ngraham wrote:
  
  > 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.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: broulik, alexeymin, IlyaBizyaev, ndavis, ngraham, kde-frameworks-devel, 
LeGast00n, cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: alexeymin, IlyaBizyaev, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Alexey Min]

alexeymin added a comment.


  F8182957: изображение.png 
  
  Without badge... Same problem? I'm using dark plasma theme (but not 
Breeze-dark specifically, it is Sweet KDE) and normal Breeze for apps

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: alexeymin, IlyaBizyaev, ndavis, ngraham, kde-frameworks-devel, LeGast00n, 
cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Noah Davis]

ndavis added a comment.


  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.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: IlyaBizyaev, ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Noah Davis]

ndavis added a comment.


  In D27617#629023 , @IlyaBizyaev 
wrote:
  
  > This breaks for people using Breeze for apps and Breeze Dark for Plasma:
  >  F8180783: image.png 
  >
  > > 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 upstream?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: IlyaBizyaev, ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Ilya Bizyaev]

IlyaBizyaev added a comment.


  This breaks for people using Breeze for apps and Breeze Dark for Plasma:
  F8180783: image.png 
  
  > 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.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: IlyaBizyaev, ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, 
GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Noah Davis]

This revision was automatically updated to reflect the committed changes.
Closed by commit R266:27740ce26135: [breeze-icons] Add telegram-desktop tray 
icons (authored by rocka, committed by ndavis).

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27617?vs=76987=76988

REVISION DETAIL
  https://phabricator.kde.org/D27617

AFFECTED FILES
  icons-dark/apps/22/telegram-attention-panel.svg
  icons-dark/apps/22/telegram-mute-panel.svg
  icons-dark/apps/22/telegram-panel.svg
  icons/apps/22/telegram-attention-panel.svg
  icons/apps/22/telegram-mute-panel.svg
  icons/apps/22/telegram-panel.svg

To: rocka, #vdg, Fuchs, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Noah Davis]

ndavis updated this revision to Diff 76987.
ndavis edited the test plan for this revision.
ndavis added a comment.


  Update author info

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27617?vs=76946=76987

BRANCH
  arcpatch-D27617_1

REVISION DETAIL
  https://phabricator.kde.org/D27617

AFFECTED FILES
  icons-dark/apps/22/telegram-attention-panel.svg
  icons-dark/apps/22/telegram-mute-panel.svg
  icons-dark/apps/22/telegram-panel.svg
  icons/apps/22/telegram-attention-panel.svg
  icons/apps/22/telegram-mute-panel.svg
  icons/apps/22/telegram-panel.svg

To: rocka, #vdg, Fuchs, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Noah Davis]

ndavis accepted this revision.
ndavis added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs, ndavis
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Rocket Aaron]

rocka updated this revision to Diff 76946.
rocka added a comment.


  Fix attention dot color and remove accidentally added patch file

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27617?vs=76942=76946

REVISION DETAIL
  https://phabricator.kde.org/D27617

AFFECTED FILES
  icons-dark/apps/22/telegram-attention-panel.svg
  icons-dark/apps/22/telegram-mute-panel.svg
  icons-dark/apps/22/telegram-panel.svg
  icons/apps/22/telegram-attention-panel.svg
  icons/apps/22/telegram-mute-panel.svg
  icons/apps/22/telegram-panel.svg

To: rocka, #vdg, Fuchs
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Rocket Aaron]

rocka updated this revision to Diff 76942.
rocka edited the test plan for this revision.
rocka added a comment.


  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`.

REPOSITORY
  R266 Breeze Icons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27617?vs=76267=76942

REVISION DETAIL
  https://phabricator.kde.org/D27617

AFFECTED FILES
  icons-dark/apps/22/telegram-attention-panel.svg
  icons-dark/apps/22/telegram-mute-panel.svg
  icons-dark/apps/22/telegram-panel.svg
  icons/apps/22/telegram-attention-panel.svg
  icons/apps/22/telegram-mute-panel.svg
  icons/apps/22/telegram-panel.svg
  telegram-panel-breeze.patch

To: rocka, #vdg, Fuchs
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Noah Davis]

ndavis added a comment.


  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.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Rocket Aaron]

rocka added a comment.


  Are we ready for some reviews?

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Rocket Aaron]

rocka added a comment.


  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.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Noah Davis]

ndavis added a comment.


  In D27617#616925 , @ngraham wrote:
  
  > Thanks for the patch!
  >
  > But...it's blue: :)
  >
  > F8128783: Screenshot_20200224_073346.png 

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

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs
Cc: ndavis, ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Nathaniel Graham]

ngraham added a comment.


  Thanks for the patch!
  
  But...it's blue: :)
  
  F8128783: Screenshot_20200224_073346.png 


REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs
Cc: ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[David Redondo]

davidre edited the summary of this revision.
davidre added a reviewer: Fuchs.

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

To: rocka, #vdg, Fuchs
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27617: [breeze-icons] Add telegram-desktop tray icons

[Rocket Aaron]

rocka created this revision.
rocka added a reviewer: VDG.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
rocka requested review of this revision.

REVISION 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.

TEST PLAN
  Before: F8128166: Screenshot_20200224_173312.png 

  
  After:
  
  | breeze | breeze-dark |
  | F8128129: Screenshot_20200224_172135.png 
  | F8128123: 
Screenshot_20200224_172102.png    
|

REPOSITORY
  R266 Breeze Icons

REVISION DETAIL
  https://phabricator.kde.org/D27617

AFFECTED FILES
  icons-dark/apps/22/telegram-attention-panel.svg
  icons-dark/apps/22/telegram-mute-panel.svg
  icons-dark/apps/22/telegram-panel.svg
  icons/apps/22/telegram-attention-panel.svg
  icons/apps/22/telegram-mute-panel.svg
  icons/apps/22/telegram-panel.svg

To: rocka, #vdg
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns