[prev in list] [next in list] [prev in thread] [next in thread]
List: calligra-devel
Subject: Re: Review Request: Introduce KoTextRange instead of inline characters
From: Pierre Stirnweiss <pstirnweiss () googlemail ! com>
Date: 2012-10-25 8:21:15
Message-ID: CAOjaSmiDyeFGefmydi1zjho7uhKoORTu4r6+MCO1-kJSVtG3kA () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Thu, Oct 25, 2012 at 9:39 AM, Inge Wallin <inge@lysator.liu.se> wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106983/
>
> Ship it!
>
> Looks good now.
>
> Only thing was that copyright notice is still missing in some files and s=
ome files still have code that is commented away.
>
> Why don't I point out the exact places? Because I got server errors from =
the review board and it had to be fixed by removing the review and I don't =
have enough energy to go through the whole thing again. Anyway it was just =
trivialities, just go ahead and merge.
>
>
> - Inge
>
> On October 24th, 2012, 11:44 a.m., C. Boemann wrote:
> Review request for Calligra.
> By C. Boemann.
>
> *Updated Oct. 24, 2012, 11:44 a.m.*
> Description
>
> Using inline characters for things like anchors, bookmarks, annotations, =
softbreaks means that the inlinecharacter show up like invisible characters=
.
>
> This is very undesirably, and prone to all kinds of bugs. Qt has a clas t=
hat already handles this mostly correctly: QTextCursor. We just need to put=
our own handling on top.
>
> This up our requirement to qt 4.7 because we need a special feature, and =
there is even a bug in that feature i had to work around for the time being=
.
>
> I suspect lots of changes is needed before it's mergable, and expect a th=
orough review by interested parties
>
> Diffs
>
> - libs/kotext/CMakeLists.txt (8c7d976)
> - libs/kotext/KoBookmark.h (3591e9d)
> - libs/kotext/KoBookmark.cpp (c8045fe)
> - libs/kotext/KoBookmarkManager.h (08006ce)
> - libs/kotext/KoBookmarkManager.cpp (b2a4ea3)
> - libs/kotext/KoInlineTextObjectManager.h (56ce7cd)
> - libs/kotext/KoInlineTextObjectManager.cpp (e82664d)
> - libs/kotext/KoText.h (6eb02ab)
> - libs/kotext/KoTextDebug.cpp (24cb4a0)
> - libs/kotext/KoTextDocument.h (76d0a5e)
> - libs/kotext/KoTextDocument.cpp (9e8727b)
> - libs/kotext/KoTextEditor.h (67458e7)
> - libs/kotext/KoTextEditor.cpp (0c5463d)
> - libs/kotext/KoTextInlineRdf.cpp (eaf0ff7)
> - libs/kotext/KoTextRange.h (PRE-CREATION)
> - libs/kotext/KoTextRange.cpp (PRE-CREATION)
> - libs/kotext/KoTextRangeManager.h (PRE-CREATION)
> - libs/kotext/KoTextRangeManager.cpp (PRE-CREATION)
> - libs/kotext/commands/DeleteCommand.h (036914b)
> - libs/kotext/commands/DeleteCommand.cpp (d95b806)
> - libs/kotext/opendocument/KoTextLoader.cpp (e373785)
> - libs/kotext/opendocument/KoTextWriter_p.cpp (7667d8e)
> - libs/kotext/tests/TestKoBookmarkManager.h (131eea7)
> - libs/kotext/tests/TestKoBookmarkManager.cpp (24d27f8)
> - libs/kotext/tests/TestKoInlineTextObjectManager.h (21c6ff9)
> - libs/kotext/tests/TestKoInlineTextObjectManager.cpp (0c606b9)
> - libs/kotext/tests/TestKoTextEditor.cpp (dde79cd)
> - libs/main/rdf/KoDocumentRdf.cpp (dfbaf09)
> - libs/main/tests/TestKoDocumentRdf.cpp (8fb279c)
> - libs/main/tests/rdf_test.h (778fcbb)
> - libs/main/tests/rdf_test.cpp (7550d3f)
> - libs/textlayout/KoPointedAt.h (0924f0d)
> - libs/textlayout/KoPointedAt.cpp (ac6c88a)
> - libs/textlayout/KoTextDocumentLayout.h (ee317d0)
> - libs/textlayout/KoTextDocumentLayout.cpp (ee6f31c)
> - libs/textlayout/KoTextLayoutArea.cpp (b184b79)
> - libs/textlayout/ToCGenerator.h (674a413)
> - libs/textlayout/ToCGenerator.cpp (b7585ef)
> - plugins/textshape/TextShape.h (75e985c)
> - plugins/textshape/TextShape.cpp (574c493)
> - plugins/textshape/TextShapeFactory.cpp (7e8d7b6)
> - plugins/textshape/TextTool.cpp (be9f2eb)
> - plugins/textshape/dialogs/BibliographyPreview.h (05d4560)
> - plugins/textshape/dialogs/BibliographyPreview.cpp (e555121)
> - plugins/textshape/dialogs/SimpleParagraphWidget.cpp (7246f7a)
> - plugins/textshape/dialogs/TableOfContentsPreview.h (ede723a)
> - plugins/textshape/dialogs/TableOfContentsPreview.cpp (1f24270)
> - words/part/KWDocument.h (16a760c)
> - words/part/KWDocument.cpp (ac734be)
> - words/part/KWView.cpp (65f6165)
> - words/part/frames/KWTextFrameSet.cpp (35f4d8e)
> - words/part/tests/TestKoBookmark.h (3620385)
> - words/part/tests/TestKoBookmark.cpp (cfb65bc)
> - words/part/tests/TestRdf.cpp (b1cf93c)
>
> View Diff <http://git.reviewboard.kde.org/r/106983/diff/>
>
> _______________________________________________
> calligra-devel mailing list
> calligra-devel@kde.org
> https://mail.kde.org/mailman/listinfo/calligra-devel
>
>
I haven't taken time to review in deep detail since i understood it would
not be for right now. Has the impact on undo/redo been tested? From the
initial version of DeleteCommand, I had the impression that undo/redo would
be messed up a bit. The doDelete would delete text ranges, but I have not
seen anything adding them again in the manager, for example.
I probably won't have time to do full review this evening (especially not
if we want to merge in the style sorting).
In any case, is it really wise to merge such a big refactoring so close to
release?
PierreSt
[Attachment #5 (text/html)]
<br><br><div class="gmail_quote">On Thu, Oct 25, 2012 at 9:39 AM, Inge Wallin <span \
dir="ltr"><<a href="mailto:inge@lysator.liu.se" \
target="_blank">inge@lysator.liu.se</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">
<div>
<div style="font-family:Verdana,Arial,Helvetica,Sans-Serif"><div class="im">
<table style="border:1px #c9c399 solid" width="100%" bgcolor="#f9f3c9" \
cellpadding="8"> <tbody><tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106983/" \
target="_blank">http://git.reviewboard.kde.org/r/106983/</a> </td>
</tr>
</tbody></table>
<br>
</div><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">Looks \
good now.
Only thing was that copyright notice is still missing in some files and some files \
still have code that is commented away.
Why don't I point out the exact places? Because I got server errors from the \
review board and it had to be fixed by removing the review and I don't have \
enough energy to go through the whole thing again. Anyway it was just trivialities, \
just go ahead and merge.</pre>
<br>
<p>- Inge</p><div class="im">
<br>
<p>On October 24th, 2012, 11:44 a.m., C. Boemann wrote:</p>
</div><table style="background-image:url('');background-repeat:repeat-x;border:1px \
black solid" width="100%" bgcolor="#fefadf" cellpadding="8" cellspacing="0"> \
<tbody><tr> <td><div class="im">
<div>Review request for Calligra.</div>
<div>By C. Boemann.</div>
<p style="color:grey"><i>Updated Oct. 24, 2012, 11:44 a.m.</i></p>
</div><h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Description </h1><div \
class="im"> <table style="border:1px solid #b8b5a0" width="100%" bgcolor="#ffffff" \
cellpadding="10" cellspacing="0"> <tbody><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">Using \
inline characters for things like anchors, bookmarks, annotations, softbreaks means \
that the inlinecharacter show up like invisible characters.
This is very undesirably, and prone to all kinds of bugs. Qt has a clas that already \
handles this mostly correctly: QTextCursor. We just need to put our own handling on \
top.
This up our requirement to qt 4.7 because we need a special feature, and there is \
even a bug in that feature i had to work around for the time being.
I suspect lots of changes is needed before it's mergable, and expect a thorough \
review by interested parties</pre> </td>
</tr>
</tbody></table>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Diffs </h1>
</div><div><div class="h5"><ul style="margin-left:3em;padding-left:0">
<li>libs/kotext/CMakeLists.txt <span style="color:grey">(8c7d976)</span></li>
<li>libs/kotext/KoBookmark.h <span style="color:grey">(3591e9d)</span></li>
<li>libs/kotext/KoBookmark.cpp <span style="color:grey">(c8045fe)</span></li>
<li>libs/kotext/KoBookmarkManager.h <span style="color:grey">(08006ce)</span></li>
<li>libs/kotext/KoBookmarkManager.cpp <span style="color:grey">(b2a4ea3)</span></li>
<li>libs/kotext/KoInlineTextObjectManager.h <span \
style="color:grey">(56ce7cd)</span></li>
<li>libs/kotext/KoInlineTextObjectManager.cpp <span \
style="color:grey">(e82664d)</span></li>
<li>libs/kotext/KoText.h <span style="color:grey">(6eb02ab)</span></li>
<li>libs/kotext/KoTextDebug.cpp <span style="color:grey">(24cb4a0)</span></li>
<li>libs/kotext/KoTextDocument.h <span style="color:grey">(76d0a5e)</span></li>
<li>libs/kotext/KoTextDocument.cpp <span style="color:grey">(9e8727b)</span></li>
<li>libs/kotext/KoTextEditor.h <span style="color:grey">(67458e7)</span></li>
<li>libs/kotext/KoTextEditor.cpp <span style="color:grey">(0c5463d)</span></li>
<li>libs/kotext/KoTextInlineRdf.cpp <span style="color:grey">(eaf0ff7)</span></li>
<li>libs/kotext/KoTextRange.h <span style="color:grey">(PRE-CREATION)</span></li>
<li>libs/kotext/KoTextRange.cpp <span style="color:grey">(PRE-CREATION)</span></li>
<li>libs/kotext/KoTextRangeManager.h <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>libs/kotext/KoTextRangeManager.cpp <span \
style="color:grey">(PRE-CREATION)</span></li>
<li>libs/kotext/commands/DeleteCommand.h <span \
style="color:grey">(036914b)</span></li>
<li>libs/kotext/commands/DeleteCommand.cpp <span \
style="color:grey">(d95b806)</span></li>
<li>libs/kotext/opendocument/KoTextLoader.cpp <span \
style="color:grey">(e373785)</span></li>
<li>libs/kotext/opendocument/KoTextWriter_p.cpp <span \
style="color:grey">(7667d8e)</span></li>
<li>libs/kotext/tests/TestKoBookmarkManager.h <span \
style="color:grey">(131eea7)</span></li>
<li>libs/kotext/tests/TestKoBookmarkManager.cpp <span \
style="color:grey">(24d27f8)</span></li>
<li>libs/kotext/tests/TestKoInlineTextObjectManager.h <span \
style="color:grey">(21c6ff9)</span></li>
<li>libs/kotext/tests/TestKoInlineTextObjectManager.cpp <span \
style="color:grey">(0c606b9)</span></li>
<li>libs/kotext/tests/TestKoTextEditor.cpp <span \
style="color:grey">(dde79cd)</span></li>
<li>libs/main/rdf/KoDocumentRdf.cpp <span style="color:grey">(dfbaf09)</span></li>
<li>libs/main/tests/TestKoDocumentRdf.cpp <span \
style="color:grey">(8fb279c)</span></li>
<li>libs/main/tests/rdf_test.h <span style="color:grey">(778fcbb)</span></li>
<li>libs/main/tests/rdf_test.cpp <span style="color:grey">(7550d3f)</span></li>
<li>libs/textlayout/KoPointedAt.h <span style="color:grey">(0924f0d)</span></li>
<li>libs/textlayout/KoPointedAt.cpp <span style="color:grey">(ac6c88a)</span></li>
<li>libs/textlayout/KoTextDocumentLayout.h <span \
style="color:grey">(ee317d0)</span></li>
<li>libs/textlayout/KoTextDocumentLayout.cpp <span \
style="color:grey">(ee6f31c)</span></li>
<li>libs/textlayout/KoTextLayoutArea.cpp <span \
style="color:grey">(b184b79)</span></li>
<li>libs/textlayout/ToCGenerator.h <span style="color:grey">(674a413)</span></li>
<li>libs/textlayout/ToCGenerator.cpp <span style="color:grey">(b7585ef)</span></li>
<li>plugins/textshape/TextShape.h <span style="color:grey">(75e985c)</span></li>
<li>plugins/textshape/TextShape.cpp <span style="color:grey">(574c493)</span></li>
<li>plugins/textshape/TextShapeFactory.cpp <span \
style="color:grey">(7e8d7b6)</span></li>
<li>plugins/textshape/TextTool.cpp <span style="color:grey">(be9f2eb)</span></li>
<li>plugins/textshape/dialogs/BibliographyPreview.h <span \
style="color:grey">(05d4560)</span></li>
<li>plugins/textshape/dialogs/BibliographyPreview.cpp <span \
style="color:grey">(e555121)</span></li>
<li>plugins/textshape/dialogs/SimpleParagraphWidget.cpp <span \
style="color:grey">(7246f7a)</span></li>
<li>plugins/textshape/dialogs/TableOfContentsPreview.h <span \
style="color:grey">(ede723a)</span></li>
<li>plugins/textshape/dialogs/TableOfContentsPreview.cpp <span \
style="color:grey">(1f24270)</span></li>
<li>words/part/KWDocument.h <span style="color:grey">(16a760c)</span></li>
<li>words/part/KWDocument.cpp <span style="color:grey">(ac734be)</span></li>
<li>words/part/KWView.cpp <span style="color:grey">(65f6165)</span></li>
<li>words/part/frames/KWTextFrameSet.cpp <span \
style="color:grey">(35f4d8e)</span></li>
<li>words/part/tests/TestKoBookmark.h <span style="color:grey">(3620385)</span></li>
<li>words/part/tests/TestKoBookmark.cpp <span \
style="color:grey">(cfb65bc)</span></li>
<li>words/part/tests/TestRdf.cpp <span style="color:grey">(b1cf93c)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106983/diff/" style="margin-left:3em" \
target="_blank">View Diff</a></p>
</div></div></td>
</tr>
</tbody></table>
</div>
</div>
<br>_______________________________________________<br>
calligra-devel mailing list<br>
<a href="mailto:calligra-devel@kde.org">calligra-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/calligra-devel" \
target="_blank">https://mail.kde.org/mailman/listinfo/calligra-devel</a><br> \
<br></blockquote></div><br>I haven't taken time to review in deep detail since i \
understood it would not be for right now. Has the impact on undo/redo been tested? \
From the initial version of DeleteCommand, I had the impression that undo/redo would \
be messed up a bit. The doDelete would delete text ranges, but I have not seen \
anything adding them again in the manager, for example.<br>
I probably won't have time to do full review this evening (especially not if we \
want to merge in the style sorting).<br>In any case, is it really wise to merge such \
a big refactoring so close to release?<br><br>PierreSt<br>
_______________________________________________
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