D15814: show all borders for pop up windows in a dock

[Friedrich W. H. Kossebau]

kossebau resigned from this revision.
kossebau added a comment.
This revision is now accepted and ready to land.


  No time for Plasma in the next months reserved, so withdrawing as 
reviewer/commenter to clear my todo list

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  dockBorders

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

To: mvourlakos, #plasma, broulik, davidedmundson, #vdg
Cc: kossebau, abetts, broulik, ngraham, kde-frameworks-devel, LeGast00n, 
michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Filip Fila]

filipf added a reviewer: VDG.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, broulik, davidedmundson, kossebau, #vdg
Cc: kossebau, abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Friedrich W. H. Kossebau]

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


  Actually popups from Plasma panels itself, at least with 
non-full-width-non-hide ones, now & then could use some more borders as well. 
  E.g. when popup is shown next to screen edge with non-full-width panel, where 
only parts of popup are touching the panels, other parts the void:
  F6808062: Screenshot_20190504_184342.png 

  Or when popups width is bigger than the non-full-width panel from which they 
are triggered (see also "full-screen" app window challenged the same time):
   F6808075: Screenshot_20190504_190227.png 

  
  Also does Plasma assume panels have simple straight edges which one or the 
other theme might not comply with ;)
  F6808084: Screenshot_20190504_190744.png 

  
  And I fully agree that some visual indicator to the origin of a popup, like 
the triangle, might improve experience a lot. IMHO a must for future Plasma 
styles :)
  Not only because the position of a popup can not always be aligned with the 
origin item due to place constraints.
  
  So, IMHO it would be great to have UI/UX designers go to their drawing boards 
and define some look & feel rules based on this. Which work for Plasma panel 
ideas and Latte dock ideas.
  Actually, also for "normal" programs while on it, as normal popup dialogs or 
content-shifting inline dialogs annoy me a lot :)
  
  (while on it, flagging this as Change Requested officially, given the !mask 
does not work as we all found)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, broulik, davidedmundson, kossebau
Cc: kossebau, abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[David Edmundson]

davidedmundson added a comment.


  > sorry, I dont understand what explicitly means
  
  As in "void setAlwaysShowAllBorders(bool)" (or whatever) that a user calls.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  dockBorders

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

mvourlakos added a comment.


  In D15814#343330 , @davidedmundson 
wrote:
  
  > >   first introduced at popupPlacement()
  >
  > ...by you :P
  
  
  I know :( and this broke plasma popups placement in !compositing mode.
  This is why I sent https://phabricator.kde.org/D15821
  which uses mask() in a more meaningful way both for plasma and Latte. 
  mask() in D15821  isnt used to 
distinguish plasma/docks but to place
  popups more intellengently. But in D15821 
 I need a window flag in order 
  to identify which popups can be placed according to the element trigerred 
them.
  I used Qt::Tooltip for this in D15821  
but I dont like it, I would prefer something
  which is not used that often.
  
  > Extra confusingly the comment above that line is from before the mask test 
was added, and in that comment dock means panel. 
  >  So the comment about when we're a dock explicitly excludes what you call a 
dock.  :/
  > 
  > I would have pushed for doing it explicitly. Ideally when 
outsideParentWindow was first introduced.
  
  sorry, I dont understand what explicitly means
  
  > Given we're using this mask test already, I don't like it, but if no-one 
else objects, ship it.
  >  (please also clarify comments here and line 906 to distinguish dock and 
dock)
  
  maybe is better to look first at D15821  
and decide afterwards if this should be commited now or when dialog2 arrives
  
  > Note, when Dialog gets a rewrite, we won't do it like that. I want a hint 
at the containment level that we can forward through CompactApplet.qml
  
  no problem for me, we can wait for this...

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  dockBorders

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[David Edmundson]

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


  > It is based on the assumption that by design plasma panels do not use mask()
  
  It's a big assumption. It seems more based on what the two windows happen to 
currently do rather than anything related to the topic at hand.
  Especially problematic as it's in frameworks we can't assume our code is the 
only user.
  
  >   first introduced at popupPlacement()
  
  ...by you :P
  
  Extra confusingly the comment above that line is from before the mask test 
was added, and in that comment dock means panel. 
  So the comment about when we're a dock explicitly excludes what you call a 
dock.  :/
  
  > Do you believe there is a better way to distinguish between docks and 
panels ?
  
  Implicitly? Probably not.
  
  I would have pushed for doing it explicitly. Ideally when outsideParentWindow 
was first introduced.
  
  Given we're using this mask test already, I don't like it, but if no-one else 
objects, ship it.
  (please also clarify comments here and line 906 to distinguish dock and dock)
  
  Note, when Dialog gets a rewrite, we won't do it like that. I want a hint at 
the containment level that we can forward through CompactApplet.qml

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  dockBorders

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

mvourlakos added inline comments.

INLINE COMMENTS

> davidedmundson wrote in dialog.cpp:209
> How does parentWindow->mask relate to being a dock or not?
> 
> Also we need to be a bit careful on names.
> _NET_WM_WINDOW_TYPE_DOCK is all panels, including the standard plasma one.

The idea was first introduced at popupPlacement() in dialog.cpp. 
In order to distinguish docks from panels.

It is based on the assumption that by design plasma panels do not use mask() 
because their shadows are drawn external from the window and most of the docks 
(e.g. Plank,Latte) are using masking for almost all of their operations.

Do you believe there is a better way to distinguish between docks and panels ?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> dialog.cpp:209
> +QWindow *parentWindow = q->transientParent();
> +bool inDock = parentWindow && !parentWindow->mask().isNull() && 
> KWindowSystem::compositingActive();
> +

How does parentWindow->mask relate to being a dock or not?

Also we need to be a bit careful on names.
_NET_WM_WINDOW_TYPE_DOCK is all panels, including the standard plasma one.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Nathaniel Graham]

ngraham added a comment.


  Ok, no problem!

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

mvourlakos added a comment.


  @ngraham @abetts I searched a bit the "downward-pointing triangle" case but I 
dont think it can be drawn based on the current plasma theme implementation. 
Plasma themes are svg(s) so the borders images can also have borders so it isnt 
just drawing a triangle based on the theme background color.
  
  I believe it would be better if the relevant discussion could be taken place 
at another place, in order to keep this review clean for feedback.
  
  This commit improves the situation for Latte a lot until something better is 
supported.
  The arrow case will also need full borders in order to be drawn correctly so 
this commit still has meaning.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

mvourlakos added reviewers: broulik, davidedmundson.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma, broulik, davidedmundson
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Andres Betts]

abetts added a comment.


  In D15814#85 , @mvourlakos 
wrote:
  
  > In D15814#10 , @ngraham 
wrote:
  >
  > > A big improvement! Even better would be a little downward-pointing 
triangle pointing to the thing that spawned it. Here's a visual example from 
macOS with a vertical Dock: F6291932: macOS.png 

  >
  >
  > I am not sure.
  >  Latte is trying to be as much plasma as possible and a "downward-pointing 
triangle" isnt shown any where else in plasma.
  >  So that would make Latte to look a bit out of place in Plasma.
  >  I think for now a small improvement would be enough.
  
  
  Our HIGs don't prevent it. It is also a different element than a popup or 
sliding notification.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Nathaniel Graham]

ngraham added a comment.


  A downward-pointing triangle doesn't make sense for pop-ups with a standard 
Plasma panel because pop-ups appear to slide out of panels, and they're always 
touching one another. So there is never a question about what the parent is. 
Latte has a different visual style and pop-ups are sort of floating in space, 
which is why I suggested the triangle.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

mvourlakos added a comment.


  In D15814#10 , @ngraham wrote:
  
  > A big improvement! Even better would be a little downward-pointing triangle 
pointing to the thing that spawned it. Here's a visual example from macOS with 
a vertical Dock: F6291932: macOS.png 
  
  
  I am not sure.
  Latte is trying to be as much plasma as possible and a "downward-pointing 
triangle" isnt shown any where else in plasma.
  So that would make Latte to look a bit out of place in Plasma.
  I think for now a small improvement would be enough.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

mvourlakos added a comment.


  In D15814#13 , @broulik wrote:
  
  > The idea is fine in principle but Plasma may also set a mask on its panels, 
noticeably in non-composited case for rounded corners, so when I disable 
compositing I get borders on all side for regular panel popups
  
  
  how about checking for KWindowSystem::compositingActive() ?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

mvourlakos updated this revision to Diff 42489.
mvourlakos added a comment.


  - do not consider dock for !compositing

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15814?vs=42480=42489

BRANCH
  dockBorders

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

To: mvourlakos, #plasma
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Andres Betts]

abetts added a comment.


  I like both ideas so far. Great improvement! Would it be too hard to create 
the pointer triangle? I think it make so much sense. That way the poppup 
doesn't feel like it is floating out of nowhere.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma
Cc: abetts, broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Kai Uwe Broulik]

broulik added a comment.


  The idea is fine in principle but Plasma may also set a mask on its panels, 
noticeably in non-composited case for rounded corners, so when I disable 
compositing I get borders on all side for regular panel popups

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma
Cc: broulik, ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Nathaniel Graham]

ngraham added a comment.


  A big improvement! Even better would be a little downward-pointing triangle 
pointing to the thing that spawned it. Here's a visual example from macOS with 
a vertical Dock: F6291932: macOS.png 

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

mvourlakos added a comment.


  F6291922: εικόνα.png 
  (before)
  
  F6291928: εικόνα.png 
  (after)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mvourlakos, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15814: show all borders for pop up windows in a dock

[Michail Vourlakos]

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

REVISION SUMMARY
  --it creates a much better and consistent visual effect
  when applets pop ups that are present in a dock draw
  all their borders.

TEST PLAN
  --use Latte dock in order to confirm the behavior
  --use traditional plasma panel in order to confirm
  that nothing broke

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  dockBorders

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

AFFECTED FILES
  src/plasmaquick/dialog.cpp

To: mvourlakos, #plasma
Cc: kde-frameworks-devel, michaelh, ngraham, bruns