Sorry to arrive after the battle, but I still would like to give an opinion on this one.<br>I don't think this change is going in the right direction. It mixes responsibilities between kotext lib/textshape/application even more.<br>
<br>I think we should have a clear separation of responsibilities:<br>kotext lib: handles character/string level<br>textshapes: handles lines inside a shape, given constraints of that shape<br>application: handles shapes<br>
<br>I think it is bad idea to put things like page size, page number,... inside kotext. This lib is supposed to be used by applications which are not necessarily page driven.<br>Equally, I don't think it is the responsibility of every text shape to have a track record of the other shapes. Children shapes eventually, but definitely not shapes that are unrelated. This should be at application level.<br>
<br>Now you have even more diffusion of stuff. If this is the direction you want to give to the text stuff, why are we still having a textshape and kotext lib?<br><br><br>Pierre<br><br><br><br><div class="gmail_quote">On Thu, Jan 27, 2011 at 6:13 AM, Thorsten Zachmann <span dir="ltr"><<a href="mailto:t.zachmann@zagge.de">t.zachmann@zagge.de</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div>
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;"><div class="im">
<table style="border: 1px solid rgb(201, 195, 153);" 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/100442/" target="_blank">http://git.reviewboard.kde.org/r/100442/</a>
</td>
</tr>
</tbody></table>
<br>
</div><pre style="white-space: pre-wrap;">I just added some small comments mostly about the style. Would be nice if you could fix those before committing.</pre>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192); border-collapse: collapse;" width="100%" bgcolor="white" border="0">
<thead>
<tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192, 192, 192); padding: 4px 8px; font-size: 9pt; text-align: left;" bgcolor="#f0f0f0">
<a href="http://git.reviewboard.kde.org/r/100442/diff/1/?file=7551#file7551line56" style="color: black; font-weight: bold; text-decoration: underline;" target="_blank">plugins/textshape/Layout.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="padding: 4px 8px; background-color: rgb(228, 217, 203); text-align: center;">
<tr>
<td colspan="4"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;">class ToCGenerator;</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);" align="right" bgcolor="#b1ebb0"><font size="2"></font></th>
<td width="50%" bgcolor="#c5ffc4"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"></pre></td>
<th style="border-left: 1px solid rgb(192, 192, 192); border-right: 1px solid rgb(192, 192, 192);" align="right" bgcolor="#b1ebb0"><font size="2">56</font></th>
<td width="50%" bgcolor="#c5ffc4"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"> <span>~</span><span>Layout</span><span>();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap;">the destructor should be marked virtual</pre>
</div>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192); border-collapse: collapse;" width="100%" bgcolor="white" border="0">
<thead>
<tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192, 192, 192); padding: 4px 8px; font-size: 9pt; text-align: left;" bgcolor="#f0f0f0">
<a href="http://git.reviewboard.kde.org/r/100442/diff/1/?file=7553#file7553line28" style="color: black; font-weight: bold; text-decoration: underline;" target="_blank">plugins/textshape/TextAnchorStrategy.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="padding: 4px 8px; background-color: rgb(228, 217, 203); text-align: center;">
<tr>
<td colspan="2"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"></pre></td>
<td colspan="2"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;">class KoTextShapeData;</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);" align="right" bgcolor="#b1ebb0"><font size="2"></font></th>
<td width="50%" bgcolor="#c5ffc4"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"></pre></td>
<th style="border-left: 1px solid rgb(192, 192, 192); border-right: 1px solid rgb(192, 192, 192);" align="right" bgcolor="#b1ebb0"><font size="2">28</font></th>
<td width="50%" bgcolor="#c5ffc4"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"><span>class</span> <span>TextAnchorStrategy</span> <span>:</span> <span>public</span> <span>KoAnchorStrategy</span> <span>{</span></pre>
</td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap;">the opening { should be moved to the next line</pre>
</div>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192); border-collapse: collapse;" width="100%" bgcolor="white" border="0">
<thead>
<tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192, 192, 192); padding: 4px 8px; font-size: 9pt; text-align: left;" bgcolor="#f0f0f0">
<a href="http://git.reviewboard.kde.org/r/100442/diff/1/?file=7554#file7554line335" style="color: black; font-weight: bold; text-decoration: underline;" target="_blank">plugins/textshape/TextAnchorStrategy.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="padding: 4px 8px; background-color: rgb(228, 217, 203); text-align: center;">
<tr>
<td colspan="2"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"></pre></td>
<td colspan="2"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;">bool TextAnchorStrategy::countVerticalRel(QRectF &anchorBoundingRect, QRectF containerBoundingRect,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);" align="right" bgcolor="#b1ebb0"><font size="2"></font></th>
<td width="50%" bgcolor="#c5ffc4"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"></pre></td>
<th style="border-left: 1px solid rgb(192, 192, 192); border-right: 1px solid rgb(192, 192, 192);" align="right" bgcolor="#b1ebb0"><font size="2">335</font></th>
<td width="50%" bgcolor="#c5ffc4"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"> <span>switch</span> <span>(</span><span>m_anchor</span><span>-></span><span>verticalRel</span><span>())</span> <span>{</span></pre>
</td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap;">the indention of the switch and case is wrong here</pre>
</div>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192); border-collapse: collapse;" width="100%" bgcolor="white" border="0">
<thead>
<tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192, 192, 192); padding: 4px 8px; font-size: 9pt; text-align: left;" bgcolor="#f0f0f0">
<a href="http://git.reviewboard.kde.org/r/100442/diff/1/?file=7559#file7559line302" style="color: black; font-weight: bold; text-decoration: underline;" target="_blank">words/part/frames/KWTextDocumentLayout.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="padding: 4px 8px; background-color: rgb(228, 217, 203); text-align: center;">
<tr>
<td colspan="4"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;">private:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);" align="right" bgcolor="#e9eaa8"><font size="2">286</font></th>
<td width="50%" bgcolor="#fdfebc"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"> <span>// if part of page is already layouted than check if there are some anchored shapes and register them</span></pre>
</td>
<th style="border-left: 1px solid rgb(192, 192, 192); border-right: 1px solid rgb(192, 192, 192);" align="right" bgcolor="#e9eaa8"><font size="2">259</font></th>
<td width="50%" bgcolor="#fdfebc"><pre style="margin: 0pt; font-size: 8pt; line-height: 140%;"><span>// QRectF bounds = m_state->shape->boundingRect();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap;">is it worth to keep the commented out code. If so please add a comment on why it is commented out, if not please delete it. That will make it easier later to figure out why the code is commented out</pre>
</div>
<br>
<p>- Thorsten</p><div class="im">
<br>
<p>On January 25th, 2011, 2:04 p.m., Matus Hanzes wrote:</p>
</div><table style="border: 1px solid black; background-repeat: repeat-x;" width="100%" bgcolor="#fefadf" cellpadding="8" cellspacing="0">
<tbody><tr>
<td><div class="im">
<div>Review request for Calligra and Casper Boemann.</div>
<div>By Matus Hanzes.</div>
</div><p style="color: grey;"><i>Updated Jan. 25, 2011, 2:04 p.m.</i></p>
<h1 style="color: rgb(87, 80, 18); font-size: 10pt; margin-top: 1.5em;">Description </h1><div><div></div><div class="h5">
<table style="border: 1px solid rgb(184, 181, 160);" width="100%" bgcolor="#ffffff" cellpadding="10" cellspacing="0">
<tbody><tr>
<td>
<pre style="margin: 0pt; padding: 0pt; white-space: pre-wrap;">This patch moves KWAnchorStrategy into text shape.
The reason is that it is not possible to do advanced shape anchor logic outside Layout.cpp.
The main idea is to register the shapes into Layout.cpp and layout handles all the necessary things.
The registration is done in KWTextDocumentLayout::positionInlineObject where all the words dependent data are set. (pageRectangle,pageContentRectangle,pageNumber)
If the document or anchored shape changes KoTextDocumentLayout::resetInlineObject is called which resets all the shapes that are not valid and layout finds the right place for them.
Any comments are welcome
</pre>
</td>
</tr>
</tbody></table>
<h1 style="color: rgb(87, 80, 18); font-size: 10pt; margin-top: 1.5em;">Diffs </h1>
<ul style="margin-left: 3em; padding-left: 0pt;">
<li>libs/flake/KoShape.h <span style="color: grey;">(f7179d7)</span></li>
<li>libs/flake/KoShape.cpp <span style="color: grey;">(c5aee86)</span></li>
<li>libs/kotext/KoTextAnchor.h <span style="color: grey;">(2bbbf9a)</span></li>
<li>libs/kotext/KoTextAnchor.cpp <span style="color: grey;">(ece23d6)</span></li>
<li>libs/kotext/KoTextDocumentLayout.h <span style="color: grey;">(4284d37)</span></li>
<li>libs/kotext/KoTextDocumentLayout.cpp <span style="color: grey;">(6b66e0f)</span></li>
<li>libs/kotext/KoTextShapeContainerModel.h <span style="color: grey;">(ce3a6ae)</span></li>
<li>libs/kotext/KoTextShapeContainerModel.cpp <span style="color: grey;">(00ca9b5)</span></li>
<li>plugins/textshape/CMakeLists.txt <span style="color: grey;">(a23ecc3)</span></li>
<li>plugins/textshape/Layout.h <span style="color: grey;">(5e42b7a)</span></li>
<li>plugins/textshape/Layout.cpp <span style="color: grey;">(e1228e4)</span></li>
<li>plugins/textshape/TextAnchorStrategy.h <span style="color: grey;">(PRE-CREATION)</span></li>
<li>plugins/textshape/TextAnchorStrategy.cpp <span style="color: grey;">(PRE-CREATION)</span></li>
<li>words/part/CMakeLists.txt <span style="color: grey;">(2d5c667)</span></li>
<li>words/part/frames/KWAnchorStrategy.h <span style="color: grey;">(b39f377)</span></li>
<li>words/part/frames/KWAnchorStrategy.cpp <span style="color: grey;">(c168962)</span></li>
<li>words/part/frames/KWTextDocumentLayout.h <span style="color: grey;">(59add4f)</span></li>
<li>words/part/frames/KWTextDocumentLayout.cpp <span style="color: grey;">(15a8803)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100442/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>