D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

ngraham added a comment.


  @ratijastk please feel free to submit another patch that does the same thing, 
but addresses @broulik's concerns. Thanks!

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Kai Uwe Broulik]

broulik added a comment.


  Fyi I reverted this change now 
https://cgit.kde.org/plasma-workspace.git/commit/?id=2594eb1b33b56ec7e0e8eb6c37acb1c7f2a5a1ff

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Kai Uwe Broulik]

broulik added a comment.


  Will you address the broken event delivery to the compact applet item?

INLINE COMMENTS

> ratijastk wrote in AbstractItem.qml:22
> Not exactly. But old layouts used to have a load of restrictions, for sure.

"Old layouts"? You only use `Layouts 1.2` features as far as I can tell

> ratijastk wrote in AbstractItem.qml:30
> Oh, give it a rest.  These two lines looking good together, this way it is 
> clear that they are an opposite of each other.

You first have to evaluate the inner statement in your head and then negate it 
vs. just seeing at a glance what it is supposed to do

> ratijastk wrote in AbstractItem.qml:74
> Not even sure, is it directly my mistake, or rebase on that later master 
> branch. Could you provide more specific numbers, please?

When I revert this patch in master the icons return to normal, so this isn't a 
rebase issue.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, LeGast00n, ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added inline comments.

INLINE COMMENTS

> broulik wrote in AbstractItem.qml:22
> You sure this high version number is neccessary?

Not exactly. But old layouts used to have a load of restrictions, for sure.

> broulik wrote in AbstractItem.qml:30
> Avoid doing negated disjunctions for clarity: `!labelVisible && !vertical`

Oh, give it a rest.  These two lines looking good together, this way it is 
clear that they are an opposite of each other.

> broulik wrote in AbstractItem.qml:74
> For some reason the icons are now smaller than they were before, observed on 
> two separate computers, they used to be the same as the panel icon here:
> F6849506: Screenshot_20190525_191014.png 
> 

Not even sure, is it directly my mistake, or rebase on that later master 
branch. Could you provide more specific numbers, please?

> broulik wrote in AbstractItem.qml:130
> Can you just make the `iconItem` have a `anchors.fill: parent`?

maaayyybe...

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

ngraham added a comment.


  @ratijastk, are you interested in sending a follow-up patch to address 
@broulik's comments?

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Kai Uwe Broulik]

broulik added a comment.


  Mouse buttons aren't propagated to the applet anymore, e.g. middle click on 
volume icon to mute no longer works.

INLINE COMMENTS

> AbstractItem.qml:22
>  import QtQuick 2.1
> +import QtQuick.Layouts 1.12
>  import org.kde.plasma.core 2.0 as PlasmaCore

You sure this high version number is neccessary?

> AbstractItem.qml:30
>  
> -height: effectiveItemSize + marginHints.top + marginHints.bottom
> -width: labelVisible ? parent.width : effectiveItemSize + 
> marginHints.left + marginHints.right
> +Layout.fillHeight: !(labelVisible || vertical)
> +Layout.fillWidth:labelVisible || vertical

Avoid doing negated disjunctions for clarity: `!labelVisible && !vertical`

> AbstractItem.qml:74
> +Layout.preferredWidth: abstractItem.effectiveItemSize
> +Layout.preferredHeight: abstractItem.effectiveItemSize
> +Layout.leftMargin: labelVisible ? marginHints.left : 0

For some reason the icons are now smaller than they were before, observed on 
two separate computers, they used to be the same as the panel icon here:
F6849506: Screenshot_20190525_191014.png 

> AbstractItem.qml:76
> +Layout.leftMargin: labelVisible ? marginHints.left : 0
> +Layout.rightMargin: labelVisible ? marginHints.right : 0
> +Layout.alignment: Qt.AlignVCenter | (labelVisible ? Qt.AlignLeft 
> : Qt.AlignHCenter)

This margin stuff doesn't work with right to left layout.
Run `plasmashell -reverse` and observe that there's no left padding in the item 
anymore:
F6849504: Screenshot_20190525_190949.png 

> AbstractItem.qml:130
> +iconItem.parent = iconItemContainer;
> +iconItem.anchors.fill = iconItemContainer;
> +}

Can you just make the `iconItem` have a `anchors.fill: parent`?

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

ngraham added a comment.


  Thanks again. Very nice patch. May it be the first of many! :)

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:a73aa33028ef: Fix system tray UI/UX  refactor 
(authored by ratijastk, committed by ngraham).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19745?vs=58123=58376

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

AFFECTED FILES
  applets/systemtray/CMakeLists.txt
  applets/systemtray/package/contents/applet/CompactApplet.qml
  applets/systemtray/package/contents/ui/ConfigEntries.qml
  applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/PlasmoidPopupsContainer.qml
  applets/systemtray/package/contents/ui/StatusNotifierItemModel.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  @davidedmundson
  
  Thanks.
  
  It is my first contribution to KDE/Plasma, so I **do hope** that I don't have 
a commit access.

INLINE COMMENTS

> davidedmundson wrote in CompactApplet.qml:34
> You will enter this branch if the system tray is on the left.
> 
> It doesn't make a difference now since Dialog gained support to not not 
> overlap panels and to switch edges if it doesn't fit.
> 
> This puts the location in a "wrong" way we hit the conflict detection in 
> Dialog and it fixes it.

Honestly, I tried putting plasma panel on each of four sides (naturally 
including left and right), but for some reason I still did not enter the 
branch. Or maybe it did, but no visual changes were observed. I don't remember.

It was not an easy decision to remove someone else's code which I do not 
understand. Whereas "(a) I do not understand; and (b) I has no visual effect" 
sounds a little bit more convincing.

As far as i get it from your reply, it's now Dialog's unified responsibility 
(which is fine, I guess) so this lines were redundant already.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  fix-system-tray

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

ngraham added a subscriber: aacid.
ngraham added a comment.


  In D19745#466760 , @davidedmundson 
wrote:
  
  > > Please, check whether 'LayoutMirroring' works properly.
  >
  > To test layout mirroring run "plasmashell --reverse"
  >  Layouts will auto handle most things, so things should be fine.
  >
  > Do you have commit access?
  
  
  BTW @aacid recently taught me that it's possible to find out this information 
for oneself by searching for a username, real name, or email address on 
https://websvn.kde.org/trunk/kde-common/accounts?view=markup.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  fix-system-tray

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: aacid, davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, 
plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[David Edmundson]

davidedmundson added a comment.


  > Please, check whether 'LayoutMirroring' works properly.
  
  To test layout mirroring run "plasmashell --reverse"
  Layouts will auto handle most things, so things should be fine.
  
  Do you have commit access?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  fix-system-tray

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> ratijastk wrote in CompactApplet.qml:34
> As far as I remember, one of the branches never got executed in my 
> environment. Could you point to specific use case which requires this "almost 
> dead" code?

You will enter this branch if the system tray is on the left.

It doesn't make a difference now since Dialog gained support to not not overlap 
panels and to switch edges if it doesn't fit.

This puts the location in a "wrong" way we hit the conflict detection in Dialog 
and it fixes it.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  fix-system-tray

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein, davidedmundson
Cc: davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  Bump (again)

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  Double checked on my system. Nothing bad happened so far.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk updated this revision to Diff 58123.
ratijastk added a comment.


  Rebase on most recent master

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19745?vs=53851=58123

BRANCH
  fix-system-tray

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

AFFECTED FILES
  applets/systemtray/CMakeLists.txt
  applets/systemtray/package/contents/applet/CompactApplet.qml
  applets/systemtray/package/contents/ui/ConfigEntries.qml
  applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/PlasmoidPopupsContainer.qml
  applets/systemtray/package/contents/ui/StatusNotifierItemModel.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  ~~Even with extra-cmake-modules 5.58, kcoreaddons needs to be updated as 
well... work in progress... please, be patient: i have autism^W slow hardware.~~
  
  5.58.0 releases just rolled out, upgrading...

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[David Redondo]

davidre added a comment.


  In D19745#465569 , @ratijastk 
wrote:
  
  > So, I've merged done git rebase on master — just one conflicting line, 
rewritten after me in a better way.  But now I have no way to test it, because 
CMake Error `Could not find a configuration file for package "ECM" that is 
compatible with requested version "5.58.0"` and even all-up-to-date Arch Linux 
does not provide those pieces. I've installed the latest CMake from the 
`testing` repository, but other versioning issues showed up.
  >
  > I'm not a big fan of submitting without checking, but I don't see any other 
way around.
  
  
  ECM is an KDE project https://api.kde.org/ecm/ and part of frameworks. Try 
installing extra-cmake-modules (don't know the exact name on arch).

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidre, davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added inline comments.

INLINE COMMENTS

> davidedmundson wrote in main.qml:66
> Not really.
> Using objectNames is a bit of an anti pattern, especially when QML has so the 
> built-in component scope hierachy.
> 
> We use it in the system tray already, and it's arguably no worse than the 
> existing applet.parent.parent.
> 
> So fine here, but only because the system is mad.

Pattern or not — Qt provides us with this `objectName` property so we can do 
stuff. But Qt itself is not engaged in providing further support for it. That's 
frustrating.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  So, I've merged done git rebase on master — just one conflicting line, 
rewritten after me in a better way.  But now I have no way to test it, because 
CMake Error `Could not find a configuration file for package "ECM" that is 
compatible with requested version "5.58.0"` and even all-up-to-date Arch Linux 
does not provide those pieces. I've installed the latest CMake from the 
`testing` repository, but other versioning issues showed up.
  
  I'm not a big fan of submitting without checking, but I don't see any other 
way around.

INLINE COMMENTS

> davidedmundson wrote in CompactApplet.qml:34
> When you say it did nothing, You'd need to test every hidden applet whilst 
> having the panel on the left.
> 
> See D1253 
> 
> If it ain't broke...

As far as I remember, one of the branches never got executed in my environment. 
Could you point to specific use case which requires this "almost dead" code?

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> davidedmundson wrote in PlasmoidItem.qml:45
> Huh? how did this work before?

Applets do it by itself, when it not do, it does not work like Weather Widget 
embed in systray.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[David Edmundson]

davidedmundson added a comment.


  I love use of layouts for everything, so in general ++

INLINE COMMENTS

> CompactApplet.qml:34
>  subText: plasmoid.toolTipSubText
> -location: if (plasmoid.parent && plasmoid.parent.parent.objectName === 
> "hiddenTasksColumn" && plasmoid.location !== PlasmaCore.Types.LeftEdge) {
> -return PlasmaCore.Types.RightEdge;

When you say it did nothing, You'd need to test every hidden applet whilst 
having the panel on the left.

See D1253 

If it ain't broke...

> PlasmoidItem.qml:45
>  if (applet && mouse.button === Qt.LeftButton) {
> -applet.expanded = true;
> +applet.expanded = !applet.expanded;
>  }

Huh? how did this work before?

> main.qml:66
>  
> +// Shouldn't it be part of Qt?
> +function findParentNamed(object, objectName) {

Not really.
Using objectNames is a bit of an anti pattern, especially when QML has so the 
built-in component scope hierachy.

We use it in the system tray already, and it's arguably no worse than the 
existing applet.parent.parent.

So fine here, but only because the system is mad.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: davidedmundson, ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, 
GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  Oh, nevermind. I forgot I should be using `arc` indeed.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

ngraham added a comment.


  What you need to do is rebase the branch for your patch on current master: 
`git pull --rebase origin master`
  
  Then fix up the merge  conflicts. Then run `arc diff` again.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  My parent commit was `169238848136`;  current master is `f8d5e1a2`. While on 
master, I am doing `git diff --summary  169238848136`. There is no intersection 
in changed files with my pull request. Still patch (as you aforementioned) can 
not be applied to master without tweaking.
  
  Am I missing something? Is `git diff` hiding something?

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

ngraham added a comment.


  I will review this, sorry it's taken so much time.
  
  I had to solve a minor merge conflict when rebasing this on master. Could you 
do that and update the patch? Thanks!

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  I'm afraid the more we wait, the more code base is diverging, which will 
require more patches for this pull request to be accepted. Btw, I'm still stuck 
with pinning old version in my package manager.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

ngraham added reviewers: mart, hein.
ngraham added a comment.


  A few more days of waiting may be required since most of the Plasma 
developers are quite busy right now. But it will get reviewed, don't worry!

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik, mart, hein
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  Hello,
  
  Any progress on this one? Does it usually take long to review?
  
  Sorry for impatience.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  In D19745#430958 , @ngraham wrote:
  
  > Pretty nice stuff! While you're at it, how about implementing a UI change 
so that while the popup is open, its contents change as you slide the mouse 
along the icons (but only when there's a single row/column)?
  
  
  Only as an optional feature. Will it be usable, though? Anyway, it'll be 
separate pull request. Let me see if I can manage my time. this week.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[Nathaniel Graham]

ngraham added a comment.


  Pretty nice stuff! While you're at it, how about implementing a UI change so 
that while the popup is open, its contents change as you slide the mouse along 
the icons (but only when there's a single row/column)?

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik
Cc: ngraham, ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D19745: Fix system tray UI/UX & refactor

[Noah Davis]

ndavis added a comment.


  +1 to this. Fitts' law comes to mind.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik
Cc: ndavis, anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, Pitel, 
ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk added a comment.


  In D19745#430695 , @anthonyfieroni 
wrote:
  
  > Video or screenshot will be helpful.
  
  
  Here they are, "debug" mode via colored rectangles with `anchors.fill: 
parent`.
  
  F6690427: old.mp4 
  F6690428: new.mp4 

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik
Cc: anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[Anthony Fieroni]

anthonyfieroni added reviewers: VDG, Plasma, broulik.
anthonyfieroni added a comment.


  Video or screenshot will be helpful.

REPOSITORY
  R120 Plasma Workspace

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

To: ratijastk, #vdg, #plasma, broulik
Cc: anthonyfieroni, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D19745: Fix system tray UI/UX & refactor

[ivan tkachenko]

ratijastk created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ratijastk requested review of this revision.

REVISION SUMMARY
  System tray widget had the following UX problem:
  Icons are laid out in a Flow QML layout, using minimal amount of
  space, thus not filling the height/width of the task bar. In other
  words: user can only click directly on an icon, not over or under it.
  
  Consider the following scenario:
  
  Given icon size X and task bar of height 1.5 * X located at the bottom;
  User moves pointer down to the limit and tries to click the icon.
  Expected outcome: applet is activated.
  Actual outcome: nothing happens, because icon (together with mouse
  area) floats slightly above the bottom.
  
  Which is inconvenient, especially when most other widgets tend to fill
  up the space.
  
  This patch fixes aforementioned problem by refactoring layouts using
  modern GridLayout, RowLayout et al., so that icons are arranged in
  rows and columns based on their number, and each one fills up its
  cell. I also made a handful of minor internal refactorings and fixes.
  Unfortunately, due to tight coupling, almost all files needed changes
  anyway.
  
  Special note on 'CompactApplet.location': it didn't seem to affect
  anything at all, so removed it.
  
  At the end of the day no visual changes should be noticeable. Layout
  works in both vertical and horizontal form-factor an all four sides of
  the desktop.

TEST PLAN
  Please, check whether 'LayoutMirroring' works properly.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  master

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

AFFECTED FILES
  applets/systemtray/CMakeLists.txt
  applets/systemtray/package/contents/applet/CompactApplet.qml
  applets/systemtray/package/contents/ui/ConfigEntries.qml
  applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
  applets/systemtray/package/contents/ui/HiddenItemsView.qml
  applets/systemtray/package/contents/ui/PlasmoidPopupsContainer.qml
  applets/systemtray/package/contents/ui/StatusNotifierItemModel.qml
  applets/systemtray/package/contents/ui/items/AbstractItem.qml
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/package/contents/ui/main.qml

To: ratijastk
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart