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

List:       koffice-devel
Subject:    Re: Review Request: Add some basic API for text on shape
From:       "Thorsten Zachmann" <t.zachmann () zagge ! de>
Date:       2010-05-24 13:59:42
Message-ID: 20100524135942.12804.98366 () localhost
[Download RAW message or body]



> On 2010-05-22 07:54:45, Thomas Zander wrote:
> > On Saturday 22. May 2010 05.48.10 you wrote:
> > > > So, can I commit this so we can continue to work on the open issues all
> > > > together as a team?
> > > 
> > > I don't think this should go in until we are clear and have at least some
> > > experience with loading/saving code that addresses the concerns raised.
> > 
> > Ehm, how do you suggest that works?
> > If you block me from committing the frameworks where load+save should be \
> > implemented how then can the load + save be implemented? 
> > > Maybe some others have also an opinion on the topic as it really is
> > > an integral part for koffice.
> > 
> > How can others have an opinion if they can't see the code in action because you \
> > block it from being committed? 
> > > > > Also for loading it would mean that we would need to look into each
> > > > > shape to see if there is a text embedded and then add the extra text
> > > > > shape after the loading of the shape. This also adds additional work
> > > > > for every shape loaded.
> > > 
> > > How do you see that. I think that adds quite a performance penalty for all
> > > shapes that get loaded.
> > 
> > This looks like a mis-quote, you didn't include any text I wrote, only what you \
> > wrote, so I can't answer that question. 
> > > So lets summarize why I think we should not make it general as it is now.
> > > It would also mix stuff e.g. captions and text on shape if I understand you
> > > correctly. However that are completely different tasks and I don't think
> > > these should be mixed. 
> > > As they are different they need to be saved and
> > > loaded quite differently.
> > 
> > Why should there be different ways to do the same thing?  Text associated with a \
> > shape. Just because ODF saves it differently? That sounds really backwards. We \
> > have a generic system called KoShape and all things build on top of that, all in \
> > the same way with specialization code in virtual methods. This is common Object \
> > Oriented design. All I'm suggesting is to do the same. Why are you against that? \
> > I don't get it... 
> > > If we use the KoTextShapeContainer as a parent
> > > of the shapes for e.g. path shapes and pictures shape for the text on
> > > shape feature defined in odf we can make loading and saving fit perfectly
> > > into the current system without any extra work.
> > 
> > Uhm, thats *exactly* what my patch does right now... And, yes, I agree it fits \
> > perfectly in the system we have now. 
> > > This will also avoid the overhead on loading for all shapes.
> > > For captions we can use a similar way that reuses most of the shape.
> > 
> > I think there is a lot of confusion going around and I started a new 'review' \
> > thread as the old one was getting really long. I'm stuck here, you give arguments \
> > why you think this shold not go in but I can't find anything I can do to solve \
> > them. Saving is *not* part of this review and I don't have the knowledge to \
> > implement it. So why can't this code go in so we can work on the saving code?? 
> > What will have to change to allow this code to be committed?

On Saturday 22 May 2010 09:54:40 Thomas Zander wrote:
> On Saturday 22. May 2010 05.48.10 you wrote:
> > > So, can I commit this so we can continue to work on the open issues all
> > > together as a team?
> > 
> > I don't think this should go in until we are clear and have at least some
> > experience with loading/saving code that addresses the concerns raised.
> 
> Ehm, how do you suggest that works?
> If you block me from committing the frameworks where load+save should be
> implemented how then can the load + save be implemented?

I think this can be committed if the concernce raised have been addressed.

> 
> > Maybe some others have also an opinion on the topic as it really is
> > an integral part for koffice.
> 
> How can others have an opinion if they can't see the code in action because
> you block it from being committed?

Everybody can look at the code and test it out. If you address the points I made
I will surely help with implementing loading and saving of the text on shape as \
defined in ODF.

> > > > Also for loading it would mean that we would need to look into each
> > > > shape to see if there is a text embedded and then add the extra text
> > > > shape after the loading of the shape. This also adds additional work
> > > > for every shape loaded.
> > 
> > How do you see that. I think that adds quite a performance penalty for
> > all shapes that get loaded.
> 
> This looks like a mis-quote, you didn't include any text I wrote, only what
> you wrote, so I can't answer that question.

This was a question posted in my post before the this one where you did not reply
to. Due to this I thought I bring it up again as I did not see your response. I
looked again and couldn't find any answer from you about that so maybe you can 
point me to your answer if you have given it already or write an answer about it.

> 
> > So lets summarize why I think we should not make it general as it is now.
> > It would also mix stuff e.g. captions and text on shape if I understand
> > you correctly. However that are completely different tasks and I don't
> > think these should be mixed.
> > As they are different they need to be saved and
> > loaded quite differently.
> 
> Why should there be different ways to do the same thing?  Text associated
> with a shape. Just because ODF saves it differently? That sounds really
> backwards. We have a generic system called KoShape and all things build on
> top of that, all in the same way with specialization code in virtual
> methods. This is common Object Oriented design. All I'm suggesting is to
> do the same. Why are you against that? I don't get it...

KOffice is an application that uses ODF as it's default format. Therefore we
should consider ODF loading and saving when designing it. Having support for
loading and saving captions which was your other usecase requites quite 
different loading/saving code. So I think we should not mix that parts as that 
will result in not so nice and inefficent code. Surely we should reuse much of
the design on how a text is attached to a shape.

> > If we use the KoTextShapeContainer as a parent
> > of the shapes for e.g. path shapes and pictures shape for the text on
> > shape feature defined in odf we can make loading and saving fit perfectly
> > into the current system without any extra work.
> 
> Uhm, thats *exactly* what my patch does right now... And, yes, I agree it
> fits perfectly in the system we have now.

With parent I mean inheriting KoTextShapeContainter for the path shape and
picture shape. Sorry if that was not clear.

> > This will also avoid the overhead on loading for all shapes.
> > For captions we can use a similar way that reuses most of the shape.
> 
> I think there is a lot of confusion going around and I started a new
> 'review' thread as the old one was getting really long. I'm stuck here,
> you give arguments why you think this shold not go in but I can't find
> anything I can do to solve them. Saving is *not* part of this review and I
> don't have the knowledge to implement it. So why can't this code go in so
> we can work on the saving code??
> 
> What will have to change to allow this code to be committed?

Inherit in KoPathShape from KoTextShapeContainer. This will make loading and
saving for text on shapes very straight forward to do.

Thorsten


- Thorsten


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


On 2010-05-17 21:42:20, Thomas Zander wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3988/
> -----------------------------------------------------------
> 
> (Updated 2010-05-17 21:42:20)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> There are some options for doing 'text-on'shape'. I just looked at OpenOffice what \
> they do and searched a bit in ODF but didn't find much. So here is some basics to \
> have positioning and sizing (which is a TODO right now). 
> I'd love to have some API review, see what stuff is missing etc.
> 
> 
> Diffs
> -----
> 
> trunk/koffice/kword/part/KWView.h 1127877 
> trunk/koffice/kword/part/KWView.cpp 1127877 
> trunk/koffice/kword/part/kword.rc 1127877 
> trunk/koffice/libs/flake/CMakeLists.txt 1127877 
> trunk/koffice/libs/flake/KoTextOnShapeContainer.h PRE-CREATION 
> trunk/koffice/libs/flake/KoTextOnShapeContainer.cpp PRE-CREATION 
> trunk/koffice/libs/flake/KoTextShapeDataBase.h PRE-CREATION 
> trunk/koffice/libs/flake/KoTextShapeDataBase.cpp PRE-CREATION 
> trunk/koffice/libs/flake/KoTextShapeDataBase_p.h PRE-CREATION 
> trunk/koffice/libs/kotext/KoTextShapeData.h 1127877 
> trunk/koffice/libs/kotext/KoTextShapeData.cpp 1127877 
> 
> Diff: http://reviewboard.kde.org/r/3988/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas
> 
> 

_______________________________________________
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]