Review Request: Store anchor info directly in shapes through a simple userdata like set of methods in KoShape

[Sebastian Sauer]

> On Sept. 20, 2011, 8:59 a.m., Sebastian Sauer wrote:
> > words/part/KWOdfWriter.cpp, line 249
> > <http://git.reviewboard.kde.org/r/102666/diff/4/?file=36532#file36532line249>
> >
> >     Is that correct? I mean we now save it even if the shape is not visible on the page (aka the shape makes use of clipping and is not visible on the page). Sounds like we could end with saving the shape multiple times now.
> 
> C. Boemann wrote:
>     wouldn't we want to still save such a shape? having both test would not har if that is the case, but to me it looks like we could potentially loose data
>     
>     But even so i don't see how that becomes double saving

Well, I was thinking that maybe that method is called n times during saving and while before we just skipped the shape if not visible we now save it everytime the method is called. So, we would need to e.g. take the anchor->pageNumber() in that condition into account too. But then that's only something I wasn't sure about if needed, if not, if that could be a problem, etc. If you think that cannot be a problem and if running cstester --roundtrip doesn't show any problems then everything is fine and I just got it wrong :)


> On Sept. 20, 2011, 8:59 a.m., Sebastian Sauer wrote:
> > words/part/frames/KWFrame.h, line 133
> > <http://git.reviewboard.kde.org/r/102666/diff/4/?file=36534#file36534line133>
> >
> >     Are you sure it still returns -1 if not defined?
> >
> 
> C. Boemann wrote:
>     yes KoTextAnchor defaults to -1 on create and load

Great, thanks.


> On Sept. 20, 2011, 8:59 a.m., Sebastian Sauer wrote:
> > words/part/frames/KWFrame.h, line 181
> > <http://git.reviewboard.kde.org/r/102666/diff/4/?file=36534#file36534line181>
> >
> >     Why are you removing comments again that are *important*?
> >
> 
> C. Boemann wrote:
>     because the variable is no longer there??
>     
>     see kotextanchor.h  pageNumber() where the comment now lives (reworded a bit)

I see. Well, that question was related to my comments above cause before the comment made clear that -1 means "no page number defined" what is != "invalid page number defined" as can be seen on bug 281869.


> On Sept. 20, 2011, 8:59 a.m., Sebastian Sauer wrote:
> > words/part/frames/KWFrame.cpp, line 54
> > <http://git.reviewboard.kde.org/r/102666/diff/4/?file=36535#file36535line54>
> >
> >     hmmm... so, every frame we create and that has no anchor specified is anchored-to-page now? That is an interesting change in behavior that needs very detailed testing.
> >
> 
> C. Boemann wrote:
>     it's not really new, and this is still just used by otherframetype 
>     
>     our main text, header and footer still ignores this info (and besides pagenumber is -1 even if we did try to use the info)

Yes, I see. Thanks for clarification.


- Sebastian


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


On Sept. 19, 2011, 11:27 p.m., C. Boemann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102666/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2011, 11:27 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Summary
> -------
> 
> Stores a new class KoAnchor on shapes. It's kind of like an extra user data for applications that support anchoring.
> 
> By having a pointer from the shape to anchoring information lot's of code in words become easier to maintain.
> 
> Plus we will be able to finally support smart positioning of page anchored shapes in words
> 
> I tried to keep it out of flake, but since it was impossible to transfer that data from KoTextLoader to words without going through hoops, and we already had two methods in KoShape that I could change hold this (in effect void) pointer I chose the latter.
> 
> Tables will be able to attach it's own variation of anchoring info to KoShape too by doing it's own subclassing of KoAnchor just like kotext does with KoTextAnchor
> 
> 
> Diffs
> -----
> 
>   libs/flake/KoShape.h 55f8e97 
>   libs/flake/KoShape.cpp 1c3fcef 
>   libs/flake/KoShape_p.h d055056 
>   libs/kotext/KoTextAnchor.h 0819c9b 
>   libs/kotext/KoTextAnchor.cpp f324505 
>   libs/kotext/opendocument/KoTextLoader.cpp 6ce4695 
>   libs/kotext/opendocument/KoTextSharedLoadingData.h ddf7fbe 
>   libs/kotext/opendocument/KoTextSharedLoadingData.cpp 6066949 
>   words/part/KWOdfSharedLoadingData.h 83e6f64 
>   words/part/KWOdfSharedLoadingData.cpp 1c93e5f 
>   words/part/KWOdfWriter.cpp 4515f6b 
>   words/part/KWRootAreaProvider.cpp 6c30e28 
>   words/part/frames/KWFrame.h b783c2f 
>   words/part/frames/KWFrame.cpp a8191be 
> 
> Diff: http://git.reviewboard.kde.org/r/102666/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> C.
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110920/1d52ae1d/attachment.htm>