Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
> On March 15, 2016, 3:15 p.m., David Rosca wrote: > > Are you sure this fixes it? I've tested it now with Qt 5.5 and it just > > completely broke the skip taskbar feature (= plasma popups are shown ALWAYS > > in taskbar). I was experimenting with various workarounds too, but found > > none (other than always using Popup as window type for Plasma::Dialog > > instead of normal window). > > > > I think it is because the _NET_WM_STATE hints are reset by Qt before Expose > > event, not in Show event. But I'm not sure about this one (didn't look in > > the code). My patch however fixes it correctly :) Oh crap. I came up with an idea for a workaround after I had started a Qt patch (basically same as yours)... and I forgot to remove it before testing - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93558 --- On March 15, 2016, 2:36 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated March 15, 2016, 2:36 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93558 --- Are you sure this fixes it? I've tested it now with Qt 5.5 and it just completely broke the skip taskbar feature (= plasma popups are shown ALWAYS in taskbar). I was experimenting with various workarounds too, but found none (other than always using Popup as window type for Plasma::Dialog instead of normal window). I think it is because the _NET_WM_STATE hints are reset by Qt before Expose event, not in Show event. But I'm not sure about this one (didn't look in the code). My patch however fixes it correctly :) - David Rosca On March 15, 2016, 2:36 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated March 15, 2016, 2:36 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/ --- (Updated March 15, 2016, 3:36 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and Plasma. Changes --- Submitted with commit 42e924e7f61e7b36266e6c3ca07aa414e88f50ca by David Edmundson to branch master. Repository: plasma-framework Description --- When we show a Qt window it resets all wm_states, including the SKIP_TASKBAR state that Qt doesn't support see QXcbWindow::setNetWmStates In order to set the flag we need to do it after Qt has mapped the window. (after a showEvent) Dialog previously did this using ExposeEvent which we know will happen after show. However: 1) This is a rather random fix 2) It will be called after TaskManager has been notified of a new window By merging into the same event we can make sure the flag is set before the task manager processes the new window. This means task manager will always skip plasma popups. A better fix will obviously be patching Qt to not reset flags it doesn't know about and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. BUG: 332024 REVIEW: Diffs - src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 Diff: https://git.reviewboard.kde.org/r/127374/diff/ Testing --- Added debug in libtaskmanager, the window flags are always correct for the new window. Thanks, David Edmundson ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93531 --- Ship it! I'd say flesh out the comment a bit more to indicate this won't be needed with newer versions of Qt (so future readers don't need to do research to answer "can I delete this?"), but otherwise it's a harmless fix that helps, so +1 - Eike Hein On March 14, 2016, 6:45 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated March 14, 2016, 6:45 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
> On March 14, 2016, 7:10 p.m., Kai Uwe Broulik wrote: > > Doesn't https://codereview.qt-project.org/#/c/149013/ fix that? > > David Edmundson wrote: > Oh wow thanks. I'd just started redoing the same patch. I could have > wasted quite some time. > > David Rosca is super awesome. > > However, this is still a better way of handling the Qt 5.5 case than what > we do currently so I'd still like to merge it. > > Anthony Fieroni wrote: > I investigate to see that cause Yakuake stays in taskbar *sometimes*. > > Kai Uwe Broulik wrote: > +1 for the patch > > My comment was more saying "someone already did a Qt patch" but I was too > lazy to type that on my phone :) > > Kai Uwe Broulik wrote: > I was wondering, can't we leverage the surface created event in Qt 5.5, > Martin did sth with that in KRunner for flags? Though this is frameworks, so > ifdef is probably bad, especially if it's a short-lived thing. No, and even if we could, that doesn't really help. We want to alter something just after showEvent, just after showEvent is the right place to do it. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93512 --- On March 14, 2016, 6:45 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated March 14, 2016, 6:45 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
> On März 14, 2016, 7:10 nachm., Kai Uwe Broulik wrote: > > Doesn't https://codereview.qt-project.org/#/c/149013/ fix that? > > David Edmundson wrote: > Oh wow thanks. I'd just started redoing the same patch. I could have > wasted quite some time. > > David Rosca is super awesome. > > However, this is still a better way of handling the Qt 5.5 case than what > we do currently so I'd still like to merge it. > > Anthony Fieroni wrote: > I investigate to see that cause Yakuake stays in taskbar *sometimes*. > > Kai Uwe Broulik wrote: > +1 for the patch > > My comment was more saying "someone already did a Qt patch" but I was too > lazy to type that on my phone :) I was wondering, can't we leverage the surface created event in Qt 5.5, Martin did sth with that in KRunner for flags? Though this is frameworks, so ifdef is probably bad, especially if it's a short-lived thing. - Kai Uwe --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93512 --- On März 14, 2016, 6:45 nachm., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated März 14, 2016, 6:45 nachm.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
> On März 14, 2016, 7:10 nachm., Kai Uwe Broulik wrote: > > Doesn't https://codereview.qt-project.org/#/c/149013/ fix that? > > David Edmundson wrote: > Oh wow thanks. I'd just started redoing the same patch. I could have > wasted quite some time. > > David Rosca is super awesome. > > However, this is still a better way of handling the Qt 5.5 case than what > we do currently so I'd still like to merge it. > > Anthony Fieroni wrote: > I investigate to see that cause Yakuake stays in taskbar *sometimes*. +1 for the patch My comment was more saying "someone already did a Qt patch" but I was too lazy to type that on my phone :) - Kai Uwe --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93512 --- On März 14, 2016, 6:45 nachm., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated März 14, 2016, 6:45 nachm.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
> On Март 14, 2016, 9:10 след обяд, Kai Uwe Broulik wrote: > > Doesn't https://codereview.qt-project.org/#/c/149013/ fix that? > > David Edmundson wrote: > Oh wow thanks. I'd just started redoing the same patch. I could have > wasted quite some time. > > David Rosca is super awesome. > > However, this is still a better way of handling the Qt 5.5 case than what > we do currently so I'd still like to merge it. I investigate to see that cause Yakuake stays in taskbar *sometimes*. - Anthony --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93512 --- On Март 14, 2016, 8:45 след обяд, David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated Март 14, 2016, 8:45 след обяд) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
> On March 14, 2016, 7:10 p.m., Kai Uwe Broulik wrote: > > Doesn't https://codereview.qt-project.org/#/c/149013/ fix that? Oh wow thanks. I'd just started redoing the same patch. I could have wasted quite some time. David Rosca is super awesome. However, this is still a better way of handling the Qt 5.5 case than what we do currently so I'd still like to merge it. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93512 --- On March 14, 2016, 6:45 p.m., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated March 14, 2016, 6:45 p.m.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 127374: Fix taskbar flicking when opening Plasma popups
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127374/#review93512 --- Doesn't https://codereview.qt-project.org/#/c/149013/ fix that? - Kai Uwe Broulik On März 14, 2016, 6:45 nachm., David Edmundson wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127374/ > --- > > (Updated März 14, 2016, 6:45 nachm.) > > > Review request for KDE Frameworks and Plasma. > > > Repository: plasma-framework > > > Description > --- > > When we show a Qt window it resets all wm_states, including the > SKIP_TASKBAR state that Qt doesn't support see > QXcbWindow::setNetWmStates > > In order to set the flag we need to do it after Qt has mapped the > window. (after a showEvent) > > Dialog previously did this using ExposeEvent which we know will happen after > show. > > However: > 1) This is a rather random fix > 2) It will be called after TaskManager has been notified of a new window > > By merging into the same event we can make sure the flag is set before > the task manager processes the new window. This means task manager will > always skip plasma popups. > > A better fix will obviously be patching Qt to not reset flags it doesn't know > about > and then we can set this flag in the ctor. I shall try and do that for Qt 5.7. > > BUG: 332024 > REVIEW: > > > Diffs > - > > src/plasmaquick/dialog.cpp 56f39c0740a1e32a9588e7461dcb45aab3fe9e85 > > Diff: https://git.reviewboard.kde.org/r/127374/diff/ > > > Testing > --- > > Added debug in libtaskmanager, the window flags are always correct for the > new window. > > > Thanks, > > David Edmundson > > ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
