Review Request 128457: Fix the infamous 'dialogs show up on the Task Manager' bug once more.

[David Rosca]

> On July 15, 2016, 7:42 a.m., David Rosca wrote:
> > Does it work with Qt < 5.6.1 now?
> 
> Martin Gräßlin wrote:
>     That would be before your change? I guess no. Btw. I read your code through today in an extremely detailed manner and I'm 100 % sure, that it's absolutely correct. Good work!
> 
> Eike Hein wrote:
>     No idea: I only have Qt 5.7. But I don't think this code makes it any worse, since we're keeping the "we need this for <Qt 5.6.1" line intact, and the other change shouldn't make it /worse/.
> 
> David Rosca wrote:
>     I think it would not have the skip taskbar state at all, so the popup will be always visible in task bar. The reason is that setState is now called only before showEvent and PlatformSurfaceCreated and Qt < 5.6.1 clears the "unknown" states in showEvent.

> since we're keeping the we need this for <Qt 5.6.1

Actually no, probably my comment was misleading but for < 5.6.1 it is needed to set the state *after* showEvent. We're now only keeping the line that makes it work correctly with >= 5.6.1 (at least for some :D)


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128457/#review97421
-----------------------------------------------------------


On July 15, 2016, 7:38 a.m., Eike Hein wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128457/
> -----------------------------------------------------------
> 
> (Updated July 15, 2016, 7:38 a.m.)
> 
> 
> Review request for Plasma, Bhushan Shah, David Edmundson, David Rosca, Martin Gräßlin, and Marco Martin.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> - Initially set state (and type, and flags) in response to PlatformSurfaceCreated.
>   We know reliably this will run before the window is mapped.
> 
> - Drop the comment about removing setState() form showEvent handler, as
>   we need it to avoid state loss in this scenario:
>   <mgraesslin> the window gets mapped first time: everything is fine
>   <mgraesslin> window gets unmapped: kwin removes the state as per spec
>   <mgraesslin> qt gets change event and removes the states it doesn't care about
>   <mgraesslin> qt maps window again and sets states
>   <mgraesslin> we lost the state
>   <mgraesslin> which means we need to set the state again from our side before(!) Qt sets it
>   <mgraesslin> and before Qt maps the window
> 
> 
> Diffs
> -----
> 
>   src/plasmaquick/dialog.cpp be74067 
> 
> Diff: https://git.reviewboard.kde.org/r/128457/diff/
> 
> 
> Testing
> -------
> 
> I added debug statements to XWindowTasksModel::Private::addWindow in libtaskmanager, which runs in repsonse to KWindowSystem::windowAdded and constructs a KWindowInfo for the newly-added window. Evaluating state() & NET::SkipTaskBar there after clicking the panel button to open Kicker, without this patch, it was sometimes 'false' (i.e. not skipping the taskbar) on the initial show and on subsequent shows. Setting the state in response to PlatformSurfaceCreated seems to fix the former, and keeping the setState call in showEvent (in combination with drosca's fixes to Qt 5.6.1+) is what keeps the latter working.
> 
> 
> Thanks,
> 
> Eike Hein
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160715/484daee6/attachment.html>