Review Request 122266: split KWRootAreaProvider into 3 classes for better overview and solving text flwing between textboxes

[Inge Wallin]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122266/#review74797
-----------------------------------------------------------


This is only a first review to get you going with some more fixes. I need to look at this in detail again after I have slept.

In general it looks clear enough even if there are many places where I don't understand the code.  But my biggest problem is that the layout for a textbox is inside Words, when it is clearly useful in all applications.  And that leads to that the base class should also move out of Words.

This is, of course, unless I haven't misunderstood something important such as the purpose of these classes. In that case, feel free to add some documentation. :)


words/part/CMakeLists.txt
<https://git.reviewboard.kde.org/r/122266/#comment51834>

    These files shouldn't really be in Words, should they?  Text boxes are useful in all ODF applications.



words/part/KWRootAreaProviderBase.h
<https://git.reviewboard.kde.org/r/122266/#comment51835>

    The base class for a root area provider should hardly include a Words specific datatype.



words/part/KWRootAreaProviderBase.cpp
<https://git.reviewboard.kde.org/r/122266/#comment51836>

    This needs to be handled by a method in KoBorder.  But this is not enough, is it?  I see you take care about padding below, but you also need to take into account margin, right?



words/part/KWRootAreaProviderBase.cpp
<https://git.reviewboard.kde.org/r/122266/#comment51837>

    I don't understand this at all.  Comment, please.



words/part/KWRootAreaProviderBase.cpp
<https://git.reviewboard.kde.org/r/122266/#comment51838>

    I think instead that all layout should be in shape coordinates, i.e. from (0, 0) to (sizeX, sizeY). This handling of world coordinates inside a shape is an abomination to all that is good in linear algebra.
    
    But I understand that this is perhaps not the time and place to change that...


- Inge Wallin


On Jan. 27, 2015, 12:17 a.m., Camilla Boemann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122266/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2015, 12:17 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> split kwrootareaprovider in 3 classes for better overview and solving text flwing between textboxes
> 
> 
> Diffs
> -----
> 
>   words/part/CMakeLists.txt 386431e 
>   words/part/KWRootAreaProvider.h 54deaf3 
>   words/part/KWRootAreaProvider.cpp d26f519 
>   words/part/KWRootAreaProviderBase.h PRE-CREATION 
>   words/part/KWRootAreaProviderBase.cpp PRE-CREATION 
>   words/part/KWRootAreaProviderTextBox.h PRE-CREATION 
>   words/part/KWRootAreaProviderTextBox.cpp PRE-CREATION 
>   words/part/frames/KWFrame.cpp f88f902 
>   words/part/frames/KWTextFrameSet.h 9106758 
>   words/part/frames/KWTextFrameSet.cpp c8ccdb9 
> 
> Diff: https://git.reviewboard.kde.org/r/122266/diff/
> 
> 
> Testing
> -------
> 
> ran cstester and did some manual testing, but should eally do it again
> 
> 
> Thanks,
> 
> Camilla Boemann
> 
>

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