'Re: Review Request: Implement loading and saving for text-on-shape'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Implement loading and saving for text-on-shape
From:       "Thomas Zander" <zander () kde ! org>
Date:       2010-06-17 7:01:42
Message-ID: 20100617070142.862.10781 () localhost
[Download RAW message or body]

> On 2010-06-16 07:46:28, Thomas Zander wrote:
> > First it would be nice to have the text-on-shape separate from the new code so \
> > its not such a huge diff.  As we can easier build on top of code that is in svn \
> > and the original text on shape diff doesn't affect any apps I suggest I commit \
> > that. 
> > 
> > Looking at the design for this patch I have one big concern; the path shape has a \
> > pointer to a resource manager.  But those two data structures have a completely \
> > different life time and holding a pointer will cause problems when one the \
> > resource manager is deleted before a shape is. The change will effectively make \
> > flake much harder or impossible to be reused in other applications that don't \
> > have the same design as KOffice applications.
> 
> Thorsten Zachmann wrote:
> On Wednesday 16 June 2010 09:46:23 Thomas Zander wrote:
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > http://reviewboard.kde.org/r/4322/#review6136
> > -----------------------------------------------------------
> > 
> > 
> > First it would be nice to have the text-on-shape separate from the new code
> > so its not such a huge diff.  As we can easier build on top of code that
> > is in svn and the original text on shape diff doesn't affect any apps I
> > suggest I commit that.
> 
> As said on your review request the way it was done in your patch does not allow \
> loading and saving and therefore it should not go in. And the case that Boudewijn \
> has taken up the task to add loading and saving as a patch shows it that it is \
> perfectly possible to work on the code without being it in svn. 
> > 
> > Looking at the design for this patch I have one big concern; the path shape
> > has a pointer to a resource manager.  But those two data structures have a
> > completely different life time and holding a pointer will cause problems
> > when one the resource manager is deleted before a shape is. 
> 
> Can you tell me how this could happen?
> 
> > 
> > trunk/koffice/libs/flake/KoPathShape.h
> > <http://reviewboard.kde.org/r/4322/#comment5737>
> > 
> > I disagree that the KoPathShape *is* a KoTextOnShapeContainer.
> > The design makes a lot of useful scenarios impossible. 
> 
> This way makes loading and saving possible. As described in my reply to your review \
> request, have such a generic way does make it vary hard to save it and load it \
> without having code that will end up to be very ugly.  This patch in no ways \
> hinders to use the same concept e.g. for captions you have pointed out as another \
> use case. This will make it quite straight forward to use also in other use cases. \
> It also addresses the problems that the data e.g. name needs to be forwarded to the \
> container. 
> > For example
> > users inheriting from a KoPathShape could before set their own
> > containerModel, but now if external people do that the text no longer gets
> > positioned correctly as the internal text shape container model is exposed
> > to the public API in the path shape where it just doesn't make sense to be
> > mixed with.
> 
> That is not different then before. The model could always be changed from the \
> outside. It is still possible to set an own model or do I miss something?

> > Looking at the design for this patch I have one big concern; the path
> > shape has a pointer to a resource manager.  But those two data
> > structures have a completely different life time and holding a pointer
> > will cause problems when one the resource manager is deleted before a
> > shape is.
> 
> Can you tell me how this could happen?

One gets deleted while the other does not. Its trivial to find usecases where that \
happens. Remember;  Flake is a library, it supports many more usecases than just the \
KOffice apps.

> > trunk/koffice/libs/flake/KoPathShape.h
> > <http://reviewboard.kde.org/r/4322/#comment5737>
> > 
> > 
> > I disagree that the KoPathShape is a KoTextOnShapeContainer.
> > The design makes a lot of useful scenarios impossible. 
> 
> This way makes loading and saving possible. 

This is not being honest, it makes loading saving easier. It is possible in various \
other ways. I suggested one solution that you keep on ignoring :(

> As described in my reply to
> your review request, have such a generic way does make it vary hard to
> save it and load it without having code that will end up to be very ugly. 

We have not seen an attempt, so that it would be ugly is purely based on your guess. \
But we do see the change in this review that is forcing an inheritance model that \
*is* ugly.  A pathshape just doesn't add information to a text container. But the \
pathshape does extend (inherit from) the text container, thats wrong from a basic \
object oriented perspective and causes problems.  
> > For example
> > 
> > users inheriting from a KoPathShape could before set their own
> > containerModel, but now if external people do that the text no longer
> > gets positioned correctly as the internal text shape container model is
> > exposed to the public API in the path shape where it just doesn't make
> > sense to be mixed with.
> 
> That is not different then before. The model could always be changed from
> the outside. It is still possible to set an own model or do I miss
> something?

The model can not be changed from the outside anymore if this change gets committed.  \
If it gets changed, things break. This eliminates entirely proper usecases where you \
need a different model, but can't do that.

- Thomas

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4322/#review6136
-----------------------------------------------------------

On 2010-06-16 20:10:25, Boudewijn Rempt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4322/
> -----------------------------------------------------------
> 
> (Updated 2010-06-16 20:10:25)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> This builds on Thomas' text-on-shape patch and implements Thorsten's suggestions.
> 
> The path shape now inherits KoTextOnShapeContainer. Path shapes have to call \
> loadText/saveText (analogous to load/saveOdfAnnotations). It is not possible to use \
> the loadOdf/saveOdf of the text shape directly since that exports/saves a frame \
> around the text which is wrong.  
> There are still some interaction problems that need to be solved after merging this \
> patch with trunk. Notably: double-clicking always selects the text tool first, as \
> it is in OpenOffice, but that is not desirably for Karbon. Unless 0 is passed for \
> the KoResourceManager, all path shapes now have an embedded text shape. On adding \
> text to a newly created shape, the text is positiong wrong until the shape is \
> resized. This is something Thorsten wants to look into, as well as some other \
> issues. 
> 
> Diffs
> -----
> 
> trunk/koffice/libs/kotext/KoTextShapeData.cpp 1138803 
> trunk/koffice/libs/flake/KoTextShapeDataBase.cpp PRE-CREATION 
> trunk/koffice/libs/flake/KoTextShapeDataBase_p.h PRE-CREATION 
> trunk/koffice/libs/kotext/KoTextShapeContainerModel.h 1138803 
> trunk/koffice/libs/kotext/KoTextShapeData.h 1138803 
> trunk/koffice/libs/flake/KoTextShapeDataBase.h PRE-CREATION 
> trunk/koffice/libs/flake/KoTextOnShapeContainer_p.h PRE-CREATION 
> trunk/koffice/libs/flake/CMakeLists.txt 1138803 
> trunk/koffice/libs/flake/KoConnectionShape.cpp 1138803 
> trunk/koffice/libs/flake/KoPathShape.h 1138803 
> trunk/koffice/libs/flake/KoPathShape.cpp 1138803 
> trunk/koffice/libs/flake/KoPathShape_p.h 1138803 
> trunk/koffice/libs/flake/KoTextOnShapeContainer.h PRE-CREATION 
> trunk/koffice/libs/flake/KoTextOnShapeContainer.cpp PRE-CREATION 
> trunk/koffice/plugins/pathshapes/ellipse/EllipseShape.cpp 1138803 
> trunk/koffice/plugins/pathshapes/enhancedpath/EnhancedPathShape.cpp 1138803 
> trunk/koffice/plugins/pathshapes/enhancedpath/EnhancedPathShapeFactory.cpp 1138803 
> trunk/koffice/plugins/pathshapes/rectangle/RectangleShape.cpp 1138803 
> trunk/koffice/plugins/pathshapes/star/StarShape.cpp 1138803 
> 
> Diff: http://reviewboard.kde.org/r/4322/diff
> 
> 
> Testing
> -------
> 
> loaded and saved several test documents and tested interoperability with \
> OpenOffice. 
> 
> Thanks,
> 
> Boudewijn
> 
> 

_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel

[prev in list] [next in list] [prev in thread] [next in thread] 