Review Request: Moving anchor strategy into text shape

[Pierre Stirnweiss]

Sorry to arrive after the battle, but I still would like to give an opinion
on this one.
I don't think this change is going in the right direction. It mixes
responsibilities between kotext lib/textshape/application even more.

I think we should have a clear separation of responsibilities:
kotext lib: handles character/string level
textshapes: handles lines inside a shape, given constraints of that shape
application: handles shapes

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.
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.

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?


Pierre



On Thu, Jan 27, 2011 at 6:13 AM, Thorsten Zachmann <t.zachmann at zagge.de>wrote:

>    This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100442/
>
> I just added some small comments mostly about the style. Would be nice if you could fix those before committing.
>
>
>    plugins/textshape/Layout.h<http://git.reviewboard.kde.org/r/100442/diff/1/?file=7551#file7551line56> (Diff
> revision 1)
>
> class ToCGenerator;
>
>    56
>
>     ~Layout();
>
>   the destructor should be marked virtual
>
>
>    plugins/textshape/TextAnchorStrategy.h<http://git.reviewboard.kde.org/r/100442/diff/1/?file=7553#file7553line28> (Diff
> revision 1)
>
> class KoTextShapeData;
>
>    28
>
> class TextAnchorStrategy  : public KoAnchorStrategy {
>
>   the opening { should be moved to the next line
>
>
>    plugins/textshape/TextAnchorStrategy.cpp<http://git.reviewboard.kde.org/r/100442/diff/1/?file=7554#file7554line335> (Diff
> revision 1)
>
> bool TextAnchorStrategy::countVerticalRel(QRectF &anchorBoundingRect, QRectF containerBoundingRect,
>
>    335
>
>     switch (m_anchor->verticalRel()) {
>
>   the indention of the switch and case is wrong here
>
>
>    words/part/frames/KWTextDocumentLayout.cpp<http://git.reviewboard.kde.org/r/100442/diff/1/?file=7559#file7559line302> (Diff
> revision 1)
>
> private:
>
>   286
>
>                 // if part of page is already layouted than check if there are some anchored shapes and register them
>
> 259
>
> //                QRectF bounds = m_state->shape->boundingRect();
>
>   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
>
>
> - Thorsten
>
> On January 25th, 2011, 2:04 p.m., Matus Hanzes wrote:
>   Review request for Calligra and Casper Boemann.
> By Matus Hanzes.
>
> *Updated Jan. 25, 2011, 2:04 p.m.*
> Description
>
> 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
>
>
>   Diffs
>
>    - libs/flake/KoShape.h (f7179d7)
>    - libs/flake/KoShape.cpp (c5aee86)
>    - libs/kotext/KoTextAnchor.h (2bbbf9a)
>    - libs/kotext/KoTextAnchor.cpp (ece23d6)
>    - libs/kotext/KoTextDocumentLayout.h (4284d37)
>    - libs/kotext/KoTextDocumentLayout.cpp (6b66e0f)
>    - libs/kotext/KoTextShapeContainerModel.h (ce3a6ae)
>    - libs/kotext/KoTextShapeContainerModel.cpp (00ca9b5)
>    - plugins/textshape/CMakeLists.txt (a23ecc3)
>    - plugins/textshape/Layout.h (5e42b7a)
>    - plugins/textshape/Layout.cpp (e1228e4)
>    - plugins/textshape/TextAnchorStrategy.h (PRE-CREATION)
>    - plugins/textshape/TextAnchorStrategy.cpp (PRE-CREATION)
>    - words/part/CMakeLists.txt (2d5c667)
>    - words/part/frames/KWAnchorStrategy.h (b39f377)
>    - words/part/frames/KWAnchorStrategy.cpp (c168962)
>    - words/part/frames/KWTextDocumentLayout.h (59add4f)
>    - words/part/frames/KWTextDocumentLayout.cpp (15a8803)
>
> View Diff <http://git.reviewboard.kde.org/r/100442/diff/>
>
> _______________________________________________
> calligra-devel mailing list
> calligra-devel at kde.org
> https://mail.kde.org/mailman/listinfo/calligra-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110127/f2768c1d/attachment.htm>