[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 127216: [KStatusNotifierItem] MinimizeRestore does not "run" over the desktop on
From: Thomas_Lübking <thomas.luebking () gmail ! com>
Date: 2016-03-06 10:23:11
Message-ID: 20160306102311.6298.57998 () mimi ! kde ! org
[Download RAW message or body]
--===============5012200245868644899==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On March 3, 2016, 10:16 p.m., Thomas Lübking wrote:
> > src/kstatusnotifieritem.cpp, line 934
> > <https://git.reviewboard.kde.org/r/127216/diff/2/?file=446044#file446044line934>
> >
> > append
> > associatedWidget->setAttribute(Qt::WA_Moved);
> >
> >
> >
> > This should tell Qt to demand an explicit position and skip placement by the WM (yes, this generally \
> > works in KWin ;-)
> > However, this flag should be implicitly set by ::move() unless the point is invalid, so you might \
> > want to "qDebug() << associatedWidgetPos;" to check whether it's invalid for the failing clients.
>
> Anthony Fieroni wrote:
> Fun facts :) void KStatusNotifierItemPrivate::minimizeRestore(bool) is called only when you click on \
> tray icon i.e. if i close app with big red X it's not called => i can't store position :)
> Anthony Fieroni wrote:
> More fun facts :) associatedWidget->setGeometry(associatedWidget->frameGeometry()); works in any case ! \
> I will add path if you want, but you will not be happy to set geometry by myself :)
> Anthony Fieroni wrote:
> ^ No, extend geometry with decoration size :)
>
> Thomas Lübking wrote:
> meehhh...
>
> That means one will have to filter QEvent::Close of associatedWidget and store the position from there.
>
> Anthony Fieroni wrote:
> It's strange, about me, associatedWidget has correct frameGeometry even it's hide.
> QObject::connect(KWindowSystem::self(), &KWindowSystem::windowRemoved, q, [this](WId winId) {
> if (associatedWidget && associatedWidget->winId() == winId) {
> associatedWidgetPos = associatedWidget->frameGeometry().topLeft();
> }
> });
> Position is correct, but again not work event with Qt::WA_Moved.
>
> Anthony Fieroni wrote:
> KWindowInfo info(associatedWidget->winId(), NET::WMDesktop | NET::WMGeometry);
> info.geometry().topLeft(); <----------------------------- decoration is included again
> to filed bug?
>
> Thomas Lübking wrote:
> You may file a bug, but I frankly doubt your findings.
> (Just more or less ported my kwindowsystem tool and the values are correct ;-)
>
> => How exactly did you determine this assumption? (Just from the walking position?)
>
> Anthony Fieroni wrote:
> I can't move widget when new position not match frameGeometry().topLeft by itself.
> associatedWidgetPos = associatedWidget->geometry().topLeft(); -> can move but new position is with \
> decoration offset associatedWidgetPos = associatedWidget->pos(); -> can't move, this is wanted \
> position, not match internal wigdet frameGeometry I saw qwidget move code, Qt::WA_Moved is setted at \
> start. I saw the qwidget close code, because when i click X -> now i CAN move to pos ! Why the hell, i \
> try setAttribute(Qt::WA_QuitOnClose, false); What are attribute setted/unsetted on X click?
>
> Thomas Lübking wrote:
> You'll fail to move because the call is idempotent, ie. the positions is assumed to be the current one \
> and thus the move call is NOOP.
> Qt::WA_QuitOnClose exits the process when the last window closes, leave it alone.
Just tried, Quassel and trojitá (latter uses QSystrayIcon + QPA) actually work with the present code.
(Not checked details, but apparently minimizeRestore isn't invoked anyway??)
Trying amarok later (requires complete update for new libav deps) but would like to express my greatest \
disgrace for systrays in general and SNI in particular. The SNI daemon silently quit (not killing kded!) \
several times! during the tests and I spent 5 minutes just to figure why I suddenly wasn't getting any \
tray icon and associated widgets turned 0x0 ... this entire thing is seriously far too fragile to be \
useful. *grrrr*
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127216/#review93143
-----------------------------------------------------------
On Feb. 29, 2016, 5:18 a.m., Anthony Fieroni wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127216/
> -----------------------------------------------------------
>
> (Updated Feb. 29, 2016, 5:18 a.m.)
>
>
> Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin Klapetek.
>
>
> Bugs: 356523
> https://bugs.kde.org/show_bug.cgi?id=356523
>
>
> Repository: knotifications
>
>
> Description
> -------
>
> Store position of widget before hide it
>
>
> Diffs
> -----
>
> src/kstatusnotifieritem.cpp cf3e7b5
> src/kstatusnotifieritemprivate_p.h 8fdfd4c
>
> Diff: https://git.reviewboard.kde.org/r/127216/diff/
>
>
> Testing
> -------
>
> Tested on pixel ration = 1
>
>
> Thanks,
>
> Anthony Fieroni
>
>
--===============5012200245868644899==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; \
border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127216/">https://git.reviewboard.kde.org/r/127216/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 3rd, 2016, 10:16 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: \
collapse; margin: 2px padding: 2px;"> <thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: \
4px 8px; text-align: left;"> <a \
href="https://git.reviewboard.kde.org/r/127216/diff/2/?file=446044#file446044line934" style="color: \
black; font-weight: bold; text-decoration: underline;">src/kstatusnotifieritem.cpp</a> <span \
style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font \
size="2">932</font></th> <td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: \
140%; margin: 0; "> <span class="n">associatedWidget</span><span class="o">-></span><span \
class="n">move</span><span class="p">(</span><span class="n">info</span><span class="p">.</span><span \
class="n">frameGeometry</span><span class="p">().</span><span class="n">topLeft</span><span \
class="p">());</span> <span class="c1">// avoid placement policies</span></pre></td> <th \
bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" \
align="right"><font size="2">933</font></th> <td bgcolor="#fdfebc" width="50%"><pre style="font-size: \
8pt; line-height: 140%; margin: 0; "> <span class="n">associatedWidget</span><span \
class="o">-></span><span class="n">move</span><span class="p">(</span><span \
class="n">associatedWidgetPos</span><span class="p">);</span></pre></td> </tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">append associatedWidget->setAttribute(Qt::WA_Moved);</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This \
should tell Qt to demand an explicit position and skip placement by the WM (yes, this generally works in \
KWin ;-)</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">However, this flag should be implicitly set by ::move() unless the point is invalid, so you \
might want to "qDebug() << associatedWidgetPos;" to check whether it's invalid for the failing \
clients.</p></pre> </blockquote>
<p>On March 4th, 2016, 5:32 a.m. UTC, <b>Anthony Fieroni</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Fun facts :) void KStatusNotifierItemPrivate::minimizeRestore(bool) is \
called only when you click on tray icon i.e. if i close app with big red X it's not called => i can't \
store position :)</p></pre> </blockquote>
<p>On March 4th, 2016, 6:06 a.m. UTC, <b>Anthony Fieroni</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">More fun facts :) \
associatedWidget->setGeometry(associatedWidget->frameGeometry()); works in any case ! I will add \
path if you want, but you will not be happy to set geometry by myself :)</p></pre> </blockquote>
<p>On March 4th, 2016, 6:11 a.m. UTC, <b>Anthony Fieroni</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">^ No, extend geometry with decoration size :)</p></pre> </blockquote>
<p>On March 4th, 2016, 3:51 p.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">meehhh...</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">That means one will have to filter QEvent::Close of \
associatedWidget and store the position from there.</p></pre> </blockquote>
<p>On March 4th, 2016, 5:25 p.m. UTC, <b>Anthony Fieroni</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">It's strange, about me, associatedWidget has correct frameGeometry even \
it's hide. QObject::connect(KWindowSystem::self(), &KWindowSystem::windowRemoved, q, <a href="WId \
winId" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
normal;">this</a> {
if (associatedWidget && associatedWidget->winId() == winId) {
associatedWidgetPos = associatedWidget->frameGeometry().topLeft();
}
});
Position is correct, but again not work event with Qt::WA_Moved.</p></pre>
</blockquote>
<p>On March 5th, 2016, 8:32 a.m. UTC, <b>Anthony Fieroni</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">KWindowInfo info(associatedWidget->winId(), NET::WMDesktop | \
NET::WMGeometry); info.geometry().topLeft(); <----------------------------- decoration is included \
again to filed bug?</p></pre>
</blockquote>
<p>On March 5th, 2016, 11:06 a.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">You may file a bug, but I frankly doubt your findings. (Just more or less \
ported my kwindowsystem tool and the values are correct ;-)</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">=> How exactly did you determine this \
assumption? (Just from the walking position?)</p></pre> </blockquote>
<p>On March 6th, 2016, 4:59 a.m. UTC, <b>Anthony Fieroni</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">I can't move widget when new position not match frameGeometry().topLeft by \
itself. associatedWidgetPos = associatedWidget->geometry().topLeft(); -> can move but new position \
is with decoration offset associatedWidgetPos = associatedWidget->pos(); -> can't move, this is \
wanted position, not match internal wigdet frameGeometry I saw qwidget move code, Qt::WA_Moved is setted \
at start. I saw the qwidget close code, because when i click X -> now i CAN move to pos ! Why the \
hell, i try setAttribute(Qt::WA_QuitOnClose, false); What are attribute setted/unsetted on X \
click?</p></pre> </blockquote>
<p>On March 6th, 2016, 8:58 a.m. UTC, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">You'll fail to move because the call is idempotent, ie. the positions is \
assumed to be the current one and thus the move call is NOOP.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Qt::WA_QuitOnClose exits the process when \
the last window closes, leave it alone.</p></pre> </blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; \
white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Just tried, Quassel and trojitá (latter uses QSystrayIcon \
+ QPA) actually work with the present code. (Not checked details, but apparently minimizeRestore isn't \
invoked anyway??)</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Trying amarok later (requires complete update for new libav deps) but \
would like to express my greatest disgrace for systrays in general and SNI in particular. The SNI daemon \
silently quit (not killing kded!) several times! during the tests and I spent 5 minutes just to figure \
why I suddenly wasn't getting any tray icon and associated widgets turned 0x0 ... this entire thing is \
seriously far too fragile to be useful. <em style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: normal;">grrrr</em></p></pre> <br />
<p>- Thomas</p>
<br />
<p>On February 29th, 2016, 5:18 a.m. UTC, Anthony Fieroni wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; \
border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for KDE Frameworks, Martin Gräßlin, Thomas Lübking, and Martin Klapetek.</div>
<div>By Anthony Fieroni.</div>
<p style="color: grey;"><i>Updated Feb. 29, 2016, 5:18 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=356523">356523</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
knotifications
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid \
#b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Store position of widget before hide \
it</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Tested on pixel ration = 1</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/kstatusnotifieritem.cpp <span style="color: grey">(cf3e7b5)</span></li>
<li>src/kstatusnotifieritemprivate_p.h <span style="color: grey">(8fdfd4c)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127216/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============5012200245868644899==--
[Attachment #3 (text/plain)]
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic