Review Request 122238: KoStyleManager independence from kotext

[Inge Wallin]

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


I didn't understand all of the code, but I made some comment.

I would be much more comfortable if the ChangeStyleCommand stored the changes in the actual styles instead of the changes in the low-level internal representation that we use to apply the styles.  But I understand that that's a rest of the previous implementation and shouldn't be changed now.


libs/kotext/OdfTextTrackStyles.h
<https://git.reviewboard.kde.org/r/122238/#comment51754>

    That is a totally unsatisfying class doc.  I have read it all and I still have no clue what the class does.



libs/kotext/OdfTextTrackStyles.h
<https://git.reviewboard.kde.org/r/122238/#comment51755>

    heh, here it was.  Better move this up to the actual class doc.
    
    I also still think that the use of the term "document" is confusing in this context where it can mean either QTextDocument or ODF document. I even believe it means different things in different sentences here. Can we clarify the terminology?
    
    The last sentence confuses me more than it clarifies me. :)



libs/kotext/OdfTextTrackStyles.cpp
<https://git.reviewboard.kde.org/r/122238/#comment51757>

    So instead of a few methods in the style manager we have both a TrackStyles object and a helper class?  Is this really necessary?  The user of the word "Helper" in calligra has always struck me as very vague. Can we find a more concrete name?



libs/kotext/commands/ChangeStylesCommand.h
<https://git.reviewboard.kde.org/r/122238/#comment51759>

    This seems strange to me.  Why would a style change store a single QTextDocument? An ODF style change affects all the ODF document with several QTextDocuments.



libs/kotext/commands/ChangeStylesCommand.h
<https://git.reviewboard.kde.org/r/122238/#comment51761>

    Can we have an explanation what this struct is good for?



libs/kotext/commands/ChangeStylesMacroCommand.cpp
<https://git.reviewboard.kde.org/r/122238/#comment51762>

    This should be commented out


- Inge Wallin


On Jan. 24, 2015, 8:27 p.m., Camilla Boemann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122238/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2015, 8:27 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> Make KoStyleManager independent of rest of libs/kotext - making it possible to move it to libs/odf
> also fixes a bug that stylechanges were never applied to the document
> 
> 
> Diffs
> -----
> 
>   libs/kotext/CMakeLists.txt bfb0b2d 
>   libs/kotext/KoTextDocument.cpp 07a47dc 
>   libs/kotext/OdfTextTrackStyles.h PRE-CREATION 
>   libs/kotext/OdfTextTrackStyles.cpp PRE-CREATION 
>   libs/kotext/commands/ChangeStylesCommand.h 3762715 
>   libs/kotext/commands/ChangeStylesCommand.cpp 771caae 
>   libs/kotext/commands/ChangeStylesMacroCommand.h 340ba12 
>   libs/kotext/commands/ChangeStylesMacroCommand.cpp 66f5326 
>   libs/kotext/styles/ChangeFollower.h 5786541 
>   libs/kotext/styles/ChangeFollower.cpp 7930cfe 
>   libs/kotext/styles/KoStyleManager.h e1b400c 
>   libs/kotext/styles/KoStyleManager.cpp 86108d6 
>   libs/textlayout/KoTextShapeData.cpp c51ec6e 
>   sheets/Map.cpp a4fcc7c 
> 
> Diff: https://git.reviewboard.kde.org/r/122238/diff/
> 
> 
> Testing
> -------
> 
> only basic editing and seeing that undo/redo works and that changes are now actually appied to the document
> 
> 
> Thanks,
> 
> Camilla Boemann
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20150124/48f2e62e/attachment.htm>