D6215: Make sure size is final after showEvent

[Marco Martin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:eab4aa9909a6: Make sure size is final after showEvent 
(authored by mart).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6215?vs=15579=15682

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

AFFECTED FILES
  src/declarativeimports/core/tooltip.cpp
  src/plasmaquick/dialog.cpp

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart edited the summary of this revision.
mart edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[David Edmundson]

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

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  so, on the 5 points:
  
  1. yes, is necessary, resizing windows in their show event is definitely not 
enough, causes events to arrive to reset to the old geometry in race with the 
setgeometry done there, if it's the qpa, if it's qwindow, if it's the 
windowmanager doing that i have no idea, i have spent a week looking into that 
and really don't want to dive more in that spaghetti, more than accepting "qt 
wants sizes set before the showevent, move is fine" as a fact and adapting to it
  2. no it's not doing that anymore, all it changes is having 
syncToMainItemSize/updateLayoutParameters actually work when they are called 
with the window not visible, exactly because of 1)
  3. yeah, perhaps it should do it in a separate commit, and should be done 
everywhere a plasmashellsurface is used, or you will have wrong positions after 
hide/show that don't cause a moveevent
  4. as i explained in https://phabricator.kde.org/D6216, not strictly 
necessary, but avoids many bindings and resizes that happen when the window 
gets shown and hidden, that shouldn't be seen by the user anyways but useless 
never the less
  5. is done for a specific scenario (that is seen in the dbusnotificationtest 
manual test in knotifications) A notification is visible, and its content is 
replaced while still visible, so the actions gets removed, the window gets 
resized, all notifications gets rearranged, then actions populated again, 
window resized again, all notifications resized again. it is not strictly a 
bug, the whole procedure looks ugly and glitchy, especially if the number of 
actions after repopulating is the same, no resize or reposition of 
notifications should happen at all.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[David Edmundson]

davidedmundson added a comment.


  > no, it just means that who calls show() or the wrong setVisible() would 
just get the previous behavior of mainItem being shown only at showevent,
  
  Ok, great
  
  
  
  It's somewhat confusing as you have multiple completely independent attempts 
to solve the same problem.
  
  We have a patch now consists of:
  
  1. an early mainItem->setVisible()
  2. always updating the platform window size regardless of whether it's visible
  3. some other wayland changes (which aren't in your commit message)
  
  And we have another patch that:
  
  4. removes use of item::visible in working out window size
  5. inhibits resizing whilst we re-populate actions whilst invisible
  
  I'm after some explanation of what the problem(s) each one of those is 
solving.
  
  If 1 works, I don't see what 2 accomplishes, you're setting the platform 
window size earlier, but to something that we know is wrong.
  
  Also if 1 works, we don't need 4 or 5? Unless it's because notification does 
is doing the positioning before the show event?  At which point we could just 
fix that more normally.

INLINE COMMENTS

> dialog.cpp:1179
>  d->updateVisibility(true);
> +d->updateTheme();
>  }

what's this about?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  ping?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D6215#117498, @davidedmundson wrote:
  
  > > this will work only when the Dialog' version of setvisible is called,
  >
  > So this will break released Plasma?
  
  
  no, it just means that who calls show() or the wrong setVisible() would just 
get the previous behavior of mainItem being shown only at showevent, so with 
the potential resize/position glitch that is already in now, but setvisible 
gets called both there and in the showevent, so it wouldn't significantly break 
(just in most common case, the second setvisible would be a noop)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[David Edmundson]

davidedmundson added a comment.


  > this will work only when the Dialog' version of setvisible is called,
  
  So this will break released Plasma?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart updated this revision to Diff 15579.
mart added a comment.


  hide again mainitem when not visible
  
  show it in setVisible, that is before show.
  this will work only when the Dialog' version of setvisible is called,
  so must be paid attention when using dialog directly from C++,
  but it covers all QML usage

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D6215?vs=15462=15579

BRANCH
  master

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

AFFECTED FILES
  src/declarativeimports/core/tooltip.cpp
  src/plasmaquick/dialog.cpp

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  with this, that removes most of the properties depending from visible, i 
usually get properly sized notifications, tough every now and then the size 
still fails and a reposition is always visible right after the notification 
appears
  https://pastebin.com/ia8nLGiw

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D6215#117370, @mart wrote:
  
  > it's maybe due to the layout of actions buttons that changes (in the 
notification applet, populatepopup) but this (columnLayout computed height) 
doesn't really have effect until it's visible.
  
  
  in fact, if in the notification applet i do, right before setVisible():
  popup->property("mainItem").value()->setVisible(true);
  
  making mainitem visible just before the showevent, 
  then the look is correct.
  
  if i do that right in the showevent handler, it's too late and i get wrongly 
calculate sizes and spurious extra resize events

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  it's maybe due to the layout of actions buttons that changes (in the 
notification applet, populatepopup) but this (columnLayout computed height) 
doesn't really have effect until it's visible.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  this is how it looks under wayland, keeping the visibility change of the 
mainitem in plasma-framework and your patch in the notifications applet 
F3787256: Spectacle.Vd3087.png 

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  In https://phabricator.kde.org/D6215#116837, @mart wrote:
  
  > with patch applied on plasma-framework and notifications applet from master 
all seems to still work ok.
  >
  > so, if i understand correctly, to try with notifiaction applet with your 
https://phabricator.kde.org/D6237 on top, and this patch that keeps updating 
the visibility of the main item.
  
  
  doesn't seem to work on wayland :/

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart added a comment.


  with patch applied on plasma-framework and notifications applet from master 
all seems to still work ok.
  
  so, if i understand correctly, to try with notifiaction applet with your 
https://phabricator.kde.org/D6237 on top, and this patch that keeps updating 
the visibility of the main item.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[David Edmundson]

davidedmundson added a comment.


  Relavent follow up to the unanswer question from yesterday:
  
  why does the implicit height change when we show it
  
  It's from some code in NotificationItem.qml
  
ColumnLayout {
id: mainLayout
anchors {
 left: appIconItem.visible || imageItem.visible ? appIconItem.right 
: parent.left
  
  This changes when we show the popup, the width available show the text, that 
changes the implicitHeight of the dialog, which changes the actual height.
  
  I /think/ changing them to check whether the image is loaded rather than 
visible will fix that issue.
  I commented it out and gammaray no longer shows the implicitHeight changing

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[David Edmundson]

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


  Ok so in summary there's two changes here:
  
  - updating our window geometry regardless of whether we're visible or not
  
  Makes total sense, there's no bandwidth overhead the platform won't send 
anything if the window isn't mapped; but it means it has the right info for 
when we want it.
  
  - not setting mainItem to invisible when the dialog is hidden.
  
  We can see in gammaray all the contents now think they're visible. I'm quite 
wary of this as it was added for a reason.
  
  The window doesn't render but we do have some code paths that release 
resources, and I'm pretty sure Animations (not Animators) will continue running.
  
  ---
  
  But most crucially with this patch I don't get notifications (running 
unpatched p-w on XCB ) 
  Can you test that combo please.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart marked an inline comment as done.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas

D6215: Make sure size is final after showEvent

[Marco Martin]

mart retitled this revision from "Introduce aboutToShow() signal" to "Make sure 
size is final after showEvent".
mart edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: mart, #plasma, davidedmundson
Cc: sebas, hein, davidedmundson, plasma-devel, #frameworks, ZrenBot, spstarr, 
progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, apol, mart, lukas