[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-panel-devel
Subject: Re: Review Request: Wait for All Ignored Windows to Close in Controller
From: "Commit Hook" <null () kde ! org>
Date: 2012-02-10 8:07:27
Message-ID: 20120210080727.25942.80130 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103858/#review10478
-----------------------------------------------------------
This review has been submitted with commit 6d595cff08fee76571fba5aef23bebdd1f33a725 \
by Aaron Seigo to branch master.
- Commit Hook
On Feb. 3, 2012, 1 p.m., David Narváez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103858/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2012, 1 p.m.)
>
>
> Review request for Plasma.
>
>
> Description
> -------
>
> This patch makes the controller wait for the ignored window to actually close. In \
> bug 291075 you can see what happens when you don't: the Browse window pops, the \
> Controller queries for the Active Window, the Active Window is not tagged, so the \
> Controller kills all of its children, including the KIconDialog itself.
> Even after this patch there's still a way to trigger this bug: since we are only \
> managing a boolean flag, you can chain something like
> openWindowWithTag() -> openWindowWithoutTag() -> openWindowWithTag()
>
> and then close the last window, which will close the controller. I think the proper \
> way to manage this is to have a counter of active tagged windows, and only close \
> when it is 0.
>
> This addresses bug 291075.
> http://bugs.kde.org/show_bug.cgi?id=291075
>
>
> Diffs
> -----
>
> plasma/desktop/shell/controllerwindow.cpp 6f3064f
>
> Diff: http://git.reviewboard.kde.org/r/103858/diff/diff
>
>
> Testing
> -------
>
> 1) Click on the Activity Bar plasmoid
> 2) Change the icon of an activity
> 3) Use Custom Icon
> 4) Browse
> 5) Select an SVG file
>
> Before this patch, Plasma would crash on step 4. Notice that, on my setup, \
> selecting a PNG file would cause the controller to close (no crash, just close) and \
> the icon wouldn't change. I think I remember being able to set a PNG file before, \
> so please let me know if this is a problem caused by this patch.
>
> Thanks,
>
> David Narváez
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103858/">http://git.reviewboard.kde.org/r/103858/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This review has been \
submitted with commit 6d595cff08fee76571fba5aef23bebdd1f33a725 by Aaron Seigo to \
branch master.</pre> <br />
<p>- Commit</p>
<br />
<p>On February 3rd, 2012, 1 p.m., David Narváez wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Plasma.</div>
<div>By David Narváez.</div>
<p style="color: grey;"><i>Updated Feb. 3, 2012, 1 p.m.</i></p>
<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;">This patch makes the controller wait for the ignored window to actually \
close. In bug 291075 you can see what happens when you don't: the Browse window \
pops, the Controller queries for the Active Window, the Active Window is not tagged, \
so the Controller kills all of its children, including the KIconDialog itself.
Even after this patch there's still a way to trigger this bug: since we are only \
managing a boolean flag, you can chain something like
openWindowWithTag() -> openWindowWithoutTag() -> openWindowWithTag()
and then close the last window, which will close the controller. I think the proper \
way to manage this is to have a counter of active tagged windows, and only close when \
it is 0.</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;">1) Click on the Activity Bar plasmoid 2) Change the icon of an activity
3) Use Custom Icon
4) Browse
5) Select an SVG file
Before this patch, Plasma would crash on step 4. Notice that, on my setup, selecting \
a PNG file would cause the controller to close (no crash, just close) and the icon \
wouldn't change. I think I remember being able to set a PNG file before, so \
please let me know if this is a problem caused by this patch.</pre> </td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=291075">291075</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>plasma/desktop/shell/controllerwindow.cpp <span style="color: \
grey">(6f3064f)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103858/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic