D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  ...and now the split is complete.
  https://phabricator.kde.org/D27466

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: IlyaBizyaev, broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, 
ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  The initial patch of the three, now I've thought up a suitable way of doing 
this, has been made:
  https://phabricator.kde.org/D27465

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: IlyaBizyaev, broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, 
ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  Actually, on second thought, @ngraham I'm not even sure if step 2 would even 
be worth it on its own, if possible.
  
  On your suggestion you have it adjusted to have step 3's tablet mode checker 
in step 2. If I don't have that check in place, for step 2, then it'd just be 
how it currently is anyway on master, effectively resulting in nothing to edit 
for a patch. I'll definitely do Step 3 as a patch, though.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: IlyaBizyaev, broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, 
ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  In D27438#613097 , @ngraham wrote:
  
  > ...one to increase the spacing when in tablet mode, and another one to 
increase the spacing when in desktop mode. At this point each patch will be 
changing a different line in the function
  
  
  Due to how the current patch handles it, I'll probably have to all of step 3 
one patch since they change the same line as each other, but for sure I can 
split steps 2 and 3(1)+3(2) of your post into individual patches.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: IlyaBizyaev, broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, 
ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev abandoned this revision.
The-Feren-OS-Dev added a comment.


  Alright, I'll start splitting the patches up now. Marking this one as 
Abandoned.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: IlyaBizyaev, broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, 
ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  In D27438#613104 , @IlyaBizyaev 
wrote:
  
  > I agree with @broulik in that the spacing already feels quite excessive, or 
at least doesn't need to be increased (:
  >
  > Does this depend on DPI/panel size maybe?
  
  
  Units.smallSpacing should change depending on the DPI, from what I've heard.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: IlyaBizyaev, broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, 
ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Ilya Bizyaev]

IlyaBizyaev added a comment.


  I agree with @broulik in that the spacing already feels quite excessive, or 
at least doesn't need to be increased (:
  
  Does this depend on DPI/panel size maybe?

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: IlyaBizyaev, broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, 
ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Nathaniel Graham]

ngraham added a comment.


  
https://community.kde.org/Infrastructure/Phabricator#Marking_patches_as_dependent_on_other_patches
  
  It would probably be simplest do just do this though:
  
  1. Abandon this patch
  2. Submit a patch to refactor that logic to use a nested multi-line function 
for readability, as proposed in my comment
  3. After that patch lands, submit two more patches, one to increase the 
spacing when in tablet mode, and another one to increase the spacing when in 
desktop mode. At this point each patch will be changing a different line in the 
function
  
  Clear as mud? :)

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  How would I split it into two patches? Both patches would edit the exact same 
line.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Nathaniel Graham]

ngraham added a comment.


  This should probably be two patches at this point: one to increase the 
spacing when using tablet mode (which IMO is an uncontroversial no-brainer) and 
another to increase the spacing when in desktop mode.

INLINE COMMENTS

> main.qml:45
> +// If Kirigami.Settings.tabletMode is true, double the amount of space 
> added around the tray icons to make them easier to tap on
> +readonly property int itemSize: 
> units.roundToIconSize(Math.min(Math.min(width, height), 
> units.iconSizes[iconSizes[Math.min(iconSizes.length-1, iconSize)]])) + 
> (Kirigami.Settings.tabletMode ? Math.round(units.smallSpacing) : 
> Math.round(units.smallSpacing/2))
>  property int hiddenItemSize: units.iconSizes.smallMedium

This should probably be rewritten as an inline function as it's now very 
difficult to read. e.g.:

  readonly property int itemSize {
  if (Kirigami.Settings.tabletMode) {
  return foo;
  } else {
  [put other logic here etc]
  return bar;
  }
  }

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev updated this revision to Diff 75851.
The-Feren-OS-Dev added a comment.


  Tweaked an added comment

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27438?vs=75850=75851

BRANCH
  master

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

AFFECTED FILES
  applets/systemtray/package/contents/ui/main.qml

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  In D27438#613051 , @davidre wrote:
  
  > In D27438#613050 , 
@The-Feren-OS-Dev wrote:
  >
  > > In D27438#612719 , @davidre 
wrote:
  > >
  > > > There is now not clickable space between items:
  > > >  F8108239: grafik.png  F8108241: 
grafik.png 
  > > >  At least it should be clickable
  > >
  > >
  > > For future reference, how do you get that size examining mode in Plasma 
Desktop Workspace?
  >
  >
  > That's inside gammaray
  
  
  For some reason I couldn't get gammaray to do that over here, but I did come 
up with a new method for the patch that should increase hitboxes as well as 
icon spacing. Could you check if it does so, just to be sure?

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev updated this revision to Diff 75850.
The-Feren-OS-Dev added a comment.


  Changed method used for increasing spacing between tray icons to something 
that increases hitbox sizes as well

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27438?vs=75804=75850

BRANCH
  master

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

AFFECTED FILES
  applets/systemtray/package/contents/ui/main.qml

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[David Redondo]

davidre added a comment.


  In D27438#613050 , 
@The-Feren-OS-Dev wrote:
  
  > In D27438#612719 , @davidre 
wrote:
  >
  > > There is now not clickable space between items:
  > >  F8108239: grafik.png  F8108241: 
grafik.png 
  > >  At least it should be clickable
  >
  >
  > For future reference, how do you get that size examining mode in Plasma 
Desktop Workspace?
  
  
  That's inside gammaray

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  In D27438#612719 , @davidre wrote:
  
  > There is now not clickable space between items:
  >  F8108239: grafik.png  F8108241: 
grafik.png 
  >  At least it should be clickable
  
  
  For future reference, how do you get that size examining mode in Plasma 
Desktop Workspace?

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Kai Uwe Broulik]

broulik added a comment.


  F8108249: Screenshot_20200217_101015.PNG 

  The spacing I have here currently is already excessive imho.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: broulik, kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, 
plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[David Redondo]

davidre requested changes to this revision.
davidre added a comment.
This revision now requires changes to proceed.


  There is now not clickable space between items:
  F8108239: grafik.png  F8108241: 
grafik.png 
  At least it should be clickable

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham, davidre
Cc: kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, plasma-devel, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev updated this revision to Diff 75804.
The-Feren-OS-Dev added a comment.


  One more patch update to keep the change consistent with the rest of the code

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27438?vs=75803=75804

BRANCH
  master

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

AFFECTED FILES
  applets/systemtray/package/contents/ui/main.qml

To: The-Feren-OS-Dev, #vdg, ngraham
Cc: kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, plasma-devel, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Konrad Materka]

kmaterka added a comment.


  There is already a margin added around icons,  "units.smallSpacing / 2" if I 
remember correctly.
  IMO it is OK without additional spacing (maybe because I'm used to it?). 
Anyway, VDG should decide what is the best and what is consistent with other 
elements of Plasma.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham
Cc: kmaterka, filipf, ndavis, anthonyfieroni, davidre, ngraham, plasma-devel, 
Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Nathaniel Graham]

ngraham accepted this revision as: ngraham.
ngraham added a comment.


  Hopefully people won't murder us over one pixel. :) IMO time to finish the 
bikeshedding if VDG people are happy with it and everyone else can tolerate it.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg, ngraham
Cc: filipf, ndavis, anthonyfieroni, davidre, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev added a comment.


  Fair point, just checked that now and they're indeed the same. Got confused 
with 0.65 as 0.65 looks different to 0.5.

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg
Cc: filipf, ndavis, anthonyfieroni, davidre, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Dominic Hayes]

The-Feren-OS-Dev updated this revision to Diff 75803.
The-Feren-OS-Dev added a comment.


  Went to 0.5, because 0.6 indeed looks just like 0.5 in execution

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27438?vs=75800=75803

BRANCH
  master

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

AFFECTED FILES
  applets/systemtray/package/contents/ui/main.qml

To: The-Feren-OS-Dev, #vdg
Cc: filipf, ndavis, anthonyfieroni, davidre, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Nathaniel Graham]

ngraham added a comment.


  2*0.7 rounded down is going to be 1. Just divide `units.smallSpacing` by two 
if you want 1.
  
  But then I have to wonder... is it really worth it to add one pixel of 
spacing?

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg
Cc: filipf, ndavis, anthonyfieroni, davidre, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27438: Increase System Tray Plasmoid spacing value slightly

[Filip Fila]

filipf added a comment.


  copy paste from Telegram
  
  > fwiw I like units.smallSpacing / 2 more but it's not that big of a 
difference

REPOSITORY
  R120 Plasma Workspace

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

To: The-Feren-OS-Dev, #vdg
Cc: filipf, ndavis, anthonyfieroni, davidre, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart