[prev in list] [next in list] [prev in thread] [next in thread]
List: calligra-devel
Subject: Re: Review Request: Fix wrong position calculations in
From: "Marijn Kruisselbrink" <m.kruisselbrink () student ! tue ! nl>
Date: 2011-09-15 20:48:29
Message-ID: 20110915204829.30707.67786 () 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/102517/#review6544
-----------------------------------------------------------
Ship it!
I don't really know what the code is supposed to do either, but if it works, that's \
all that matters I'd say...
- Marijn
On Sept. 2, 2011, 8:03 a.m., Sebastian Sauer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102517/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2011, 8:03 a.m.)
>
>
> Review request for Calligra, Marijn Kruisselbrink and Stefan Nikolaus.
>
>
> Summary
> -------
>
> This fixes a crash when selecting cell after creating a chart ( bug \
> https://bugs.kde.org/show_bug.cgi?id=279951 ).
> This patch is rather essential and changes how selections are translated to a range \
> of rows and columns. Before we did;
> const KoShape* shape = \
> tool()->canvas()->shapeManager()->selection()->firstSelectedShape(); const QPointF \
> position = documentPos - (shape ? shape->position() : QPointF(0.0, 0.0));
> and now (with this patch) we do;
>
> const QPointF position = documentPos;
>
> What means we don't try to take the position of the first selected shape into \
> account any longer. That seems to work great, fixes bug 279951 and also gets right \
> of a few other asserts (e.g. select C3 and keep the mouse to expand the selected \
> range to A1 and go a bit future to have also the header in the selection => \
> assert).
> The push that added the firstSelectedShape() to Tables (back then KSpread) was \
> commit 2afef0a9;
> Author: Stefan Nikolaus <stefan.nikolaus@kdemail.net> 2008-06-14 12:40:03
> Committer: Stefan Nikolaus <stefan.nikolaus@kdemail.net> 2008-06-14 12:40:03
> Parent: 781ac3926d0e6224d4547cb5041041f66c4ecd97 (* New, updated TODO list managed \
> using emacs' org-mode)
> Cell Tool Switch to strategies for the mouse interaction.
> Drag'n'drop is special cased.
>
> I tried to compile that revision to check if the bug was present but seems I get \
> interesting compile-errors related to undefined QString, KGlobal, etc. Maybe cause \
> 2008 we where building still against Qt3? Or against a moving kdelibs (aka the \
> KDE4-port)? Not sure there but also later revisions fail to compile... I didn't \
> went future cause since that particular commit *lot* of refactoring happens and it \
> can also be the case that one of the many refactorings like e.g. commit 3fcae916 \
> did break it. I wouldn't wonder...
>
> Diffs
> -----
>
> tables/ui/AbstractSelectionStrategy.cpp f5982f9
> tables/ui/DragAndDropStrategy.cpp f500b5a
> tables/ui/SelectionStrategy.cpp 8844e47
>
> Diff: http://git.reviewboard.kde.org/r/102517/diff
>
>
> Testing
> -------
>
> I tested various drag and drop and selection cases and they all still seem to work \
> as expected. Also it fixes bug 279951 .
>
> Thanks,
>
> Sebastian
>
>
[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/102517/">http://git.reviewboard.kde.org/r/102517/</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;">I don't really know \
what the code is supposed to do either, but if it works, that's all that matters \
I'd say...</pre> <br />
<p>- Marijn</p>
<br />
<p>On September 2nd, 2011, 8:03 a.m., Sebastian Sauer 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 Calligra, Marijn Kruisselbrink and Stefan Nikolaus.</div>
<div>By Sebastian Sauer.</div>
<p style="color: grey;"><i>Updated Sept. 2, 2011, 8:03 a.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 fixes a crash when selecting cell after creating a chart ( bug \
https://bugs.kde.org/show_bug.cgi?id=279951 ).
This patch is rather essential and changes how selections are translated to a range \
of rows and columns. Before we did;
const KoShape* shape = \
tool()->canvas()->shapeManager()->selection()->firstSelectedShape(); \
const QPointF position = documentPos - (shape ? shape->position() : QPointF(0.0, \
0.0));
and now (with this patch) we do;
const QPointF position = documentPos;
What means we don't try to take the position of the first selected shape into \
account any longer. That seems to work great, fixes bug 279951 and also gets right of \
a few other asserts (e.g. select C3 and keep the mouse to expand the selected range \
to A1 and go a bit future to have also the header in the selection => assert).
The push that added the firstSelectedShape() to Tables (back then KSpread) was commit \
2afef0a9;
Author: Stefan Nikolaus <stefan.nikolaus@kdemail.net> 2008-06-14 12:40:03
Committer: Stefan Nikolaus <stefan.nikolaus@kdemail.net> 2008-06-14 \
12:40:03 Parent: 781ac3926d0e6224d4547cb5041041f66c4ecd97 (* New, updated TODO list \
managed using emacs' org-mode)
Cell Tool Switch to strategies for the mouse interaction.
Drag'n'drop is special cased.
I tried to compile that revision to check if the bug was present but seems I get \
interesting compile-errors related to undefined QString, KGlobal, etc. Maybe cause \
2008 we where building still against Qt3? Or against a moving kdelibs (aka the \
KDE4-port)? Not sure there but also later revisions fail to compile... I didn't \
went future cause since that particular commit *lot* of refactoring happens and it \
can also be the case that one of the many refactorings like e.g. commit 3fcae916 did \
break it. I wouldn't wonder... </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;">I tested various drag and drop and selection cases and they all still \
seem to work as expected. Also it fixes bug 279951 . </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>tables/ui/AbstractSelectionStrategy.cpp <span style="color: \
grey">(f5982f9)</span></li>
<li>tables/ui/DragAndDropStrategy.cpp <span style="color: \
grey">(f500b5a)</span></li>
<li>tables/ui/SelectionStrategy.cpp <span style="color: grey">(8844e47)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102517/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
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