[prev in list] [next in list] [prev in thread] [next in thread]
List: calligra-devel
Subject: Re: Review Request 123911: Fix crash due to window menu and view list not properly updating on secon
From: "Boudewijn Rempt" <boud () valdyas ! org>
Date: 2015-05-27 8:06:49
Message-ID: 20150527080649.31615.83309 () mimi ! kde ! org
[Download RAW message or body]
--===============8135283483723600724==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123911/#review80882
-----------------------------------------------------------
Ship it!
Ok, then please push :-)
- Boudewijn Rempt
On May 26, 2015, 11:09 p.m., Stefano Bonicatti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123911/
> -----------------------------------------------------------
>
> (Updated May 26, 2015, 11:09 p.m.)
>
>
> Review request for Calligra.
>
>
> Repository: calligra
>
>
> Description
> -------
>
> If two (ore more) windows are opened, then on the first multiple documents are \
> opened, all the other windows won't update their view list in the Window -> New \
> View menu. This also happens when closing documents on the first window, the other \
> will still show a possibility to open a view on a document that is actually closed, \
> and if you try to, Krita will crash.
> This happen because the secondary windows updates their menu only when receiving \
> documentOpened, documentClosed signals which are originally emitted by \
> KisPart::addView and KisPart::removeView or when a view, in the same window, is \
> activated. Given that to trigger the bug the secondary windows should be with no \
> view opened, then the issue is on the signals: documentOpened is only called if the \
> amount of views present is 1. Something similar goes for documentClosed, it is only \
> sent when there's no view opened.
> This is wrong because documentOpened should be bound to the event of actually \
> opening a new document, not a view, and no views amount check should be done. For \
> documentClosed is again the same, it should be bound to the event of actually \
> closing a document not a view. Also, adding a view
> It wasn't directly inherent with this issue, but i've also fixed the fact that all \
> new windows aren't appended in the KisPart mainWindows list.
> This might also fix: https://bugs.kde.org/show_bug.cgi?id=348162 since the \
> backtrace is very similar to the crash i got.
> While this patch is simple i preferred opening a review just to be sure i didn't \
> miss any unwanted behaviour.
>
> Diffs
> -----
>
> krita/ui/KisApplication.cpp 082ffee
> krita/ui/KisPart.cpp 24aac23
>
> Diff: https://git.reviewboard.kde.org/r/123911/diff/
>
>
> Testing
> -------
>
> Compiles fine, the view list updates correctly both in the case of opening or \
> closing a document.
>
> Thanks,
>
> Stefano Bonicatti
>
>
--===============8135283483723600724==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<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/123911/">https://git.reviewboard.kde.org/r/123911/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<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;">Ok, \
then please push :-)</p></pre> <br />
<p>- Boudewijn Rempt</p>
<br />
<p>On May 26th, 2015, 11:09 p.m. UTC, Stefano Bonicatti 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 Calligra.</div>
<div>By Stefano Bonicatti.</div>
<p style="color: grey;"><i>Updated May 26, 2015, 11:09 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
calligra
</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;">If two (ore more) windows are opened, then on the \
first multiple documents are opened, all the other windows won't update their view \
list in the Window -> New View menu. This also happens when closing documents on \
the first window, the other will still show a possibility to open a view on a \
document that is actually closed, and if you try to, Krita will crash.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">This happen because the secondary windows updates their menu only when \
receiving documentOpened, documentClosed signals which are originally emitted by \
KisPart::addView and KisPart::removeView or when a view, in the same window, is \
activated. Given that to trigger the bug the secondary windows should be with no view \
opened, then the issue is on the signals: documentOpened is only called if the amount \
of views present is 1. Something similar goes for documentClosed, it is only sent \
when there's no view opened.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">This is wrong because documentOpened \
should be bound to the event of actually opening a new document, not a view, and no \
views amount check should be done. For documentClosed is again the same, it should be \
bound to the event of actually closing a document not a view. Also, adding a view
It wasn't directly inherent with this issue, but i've also fixed the fact that all \
new windows aren't appended in the KisPart mainWindows list.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This \
might also fix: https://bugs.kde.org/show_bug.cgi?id=348162 since the backtrace is \
very similar to the crash i got.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">While this patch is \
simple i preferred opening a review just to be sure i didn't miss any unwanted \
behaviour.</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;">Compiles fine, the view list updates correctly both in \
the case of opening or closing a document.</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>krita/ui/KisApplication.cpp <span style="color: grey">(082ffee)</span></li>
<li>krita/ui/KisPart.cpp <span style="color: grey">(24aac23)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123911/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============8135283483723600724==--
[Attachment #3 (text/plain)]
_______________________________________________
calligra-devel mailing list
calligra-devel@kde.org
https://mail.kde.org/mailman/listinfo/calligra-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic