Review Request: Moving anchor strategy into text shape
Pierre Stirnweiss
pstirnweiss at googlemail.com
Thu Jan 27 10:15:22 GMT 2011
On Thu, Jan 27, 2011 at 10:51 AM, C. Boemann <cbo at boemann.dk> wrote:
> Well this is a step on the road and not the end result. So bear with me.
>
> But I've indeed changed my mind about who does the layout and drawing, and
> have come up with an abstraction that will allow the kotext library to do
> just
> that.
So you mean, drawing/layouting would be done at kotext level?
> Well totally controlled by the textshape. This abstraction will allow
> any application to gain layout and painting.
This is already the case. kotext provides an abstracted interface for
layouting. the user of the lib needs to implement his layouting logic. What
layouting logic/paradigm would you favor for kotext? this would also mean
that any other layouting paradigm/logic, would still need to carry the
weight (memory footprint,...) of a layouting logic he does not even care
about?
Sounds like a step in the opposite direction of the goal of deploying/using
on a multitude of devices, for a multitude of use cases.
A bit like MS having internet explorer right tied up with their kernel. It
becomes awefully difficult to have a very lean kernel based on windows.
First app that will benefit from
> this is our very own Tables.
>
> But shapes and pages will soon be abstracted away as i said. Oh and no one
> requires those pagesizes and numbers to be filled out by apps that don't
> use
> it. For those apps the anchoring mode that relate to these things will
> simply
> not be activated.
>
Yes, no one has to fill these, but you are bloating a general purpose
library with specific cases peculiarities. You'll end up with a melting pot
library of things which do not relate to each other. You mention Tables, so
why not also add the Spreadsheet/Cell coordinates to the mix? And since
kotext could also be used in a painting application, the layer to which the
anchored shape belongs. then, one could use kotext for a music notation
stuff, so we could add score/voice/bar information.
I am exaggerating this a lot, but once the box is opened, it is very
difficult to draw a line.
Pierre
>
> On Thursday 27 January 2011 10:25:08 Pierre Stirnweiss wrote:
> > 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/dif
> > > f/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.or
> > > g/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
> _______________________________________________
> 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/873d17db/attachment.htm>
More information about the calligra-devel
mailing list