Review Request 108459: Refactor in KoText

Fri Jan 25 04:25:45 GMT 2013

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

With the latest diff the cstester results are good. There are some nice improvements in some documents.

libs/kotext/KoAnchorInlineObject.h
<http://git.reviewboard.kde.org/r/108459/#comment19904>

    These functions should be made const

libs/kotext/KoAnchorInlineObject.h
<http://git.reviewboard.kde.org/r/108459/#comment19905>

    This function should be const.

libs/kotext/KoAnchorInlineObject.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19901>

    Please use only 4 spaces here.

libs/kotext/KoAnchorInlineObject.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19902>

    This is used.

libs/kotext/KoAnchorInlineObject.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19903>

    This is used

libs/kotext/KoAnchorTextRange.h
<http://git.reviewboard.kde.org/r/108459/#comment19906>

    the implementation should be moved to the cpp file.

libs/kotext/KoAnchorTextRange.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19907>

    4 spaces.

libs/kotext/KoAnchorTextRange.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19908>

    shouldn't the d_ptr here be deleted?

libs/kotext/KoShapeAnchor.h
<http://git.reviewboard.kde.org/r/108459/#comment19909>

    This should be const.

libs/kotext/KoShapeAnchor.h
<http://git.reviewboard.kde.org/r/108459/#comment19910>

    Can the method made const?

libs/kotext/KoShapeAnchor.h
<http://git.reviewboard.kde.org/r/108459/#comment19911>

    This should be const.

libs/kotext/KoShapeAnchor.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19912>

    The test is not needed. delete already tests this.

libs/kotext/commands/ChangeAnchorPropertiesCommand.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19913>

    Should that not delete the m_newLocation/m_oldLocation depending on which one is set when destructing to avoid a memory leak?

libs/kotext/commands/ChangeTrackedDeleteCommand.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19914>

    Maybe add a comment on why it is commented out or remove if no longer needed.

libs/kotext/opendocument/KoTextWriter_p.cpp
<http://git.reviewboard.kde.org/r/108459/#comment19915>

    Is that a change belonging to this work?

- Thorsten Zachmann

On Jan. 23, 2013, 9:35 a.m., C. Boemann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108459/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2013, 9:35 a.m.)
> 
> 
> Review request for Calligra and Thorsten Zachmann.
> 
> 
> Description
> -------
> 
>     Change KoTextAnchor into two new classes
>      KoShapeANchor - which in theory should be moved to flake later
>      KoAnchorInlineObject - which is the kotext part responsible for being an inline object
>     
>     No realy user visible changes coming out of this but it's nessesary ground work for the next step
>     
>     I have had to disable several commands so we loos that functionality
>     This will be reenabled with the following commits which adds a third class
> 
> 
> Diffs
> -----
> 
>   libs/kotext/CMakeLists.txt 8b358ec 
>   libs/kotext/KoAnchorInlineObject.h PRE-CREATION 
>   libs/kotext/KoAnchorInlineObject.cpp PRE-CREATION 
>   libs/kotext/KoAnchorTextRange.h PRE-CREATION 
>   libs/kotext/KoAnchorTextRange.cpp PRE-CREATION 
>   libs/kotext/KoShapeAnchor.h PRE-CREATION 
>   libs/kotext/KoShapeAnchor.cpp PRE-CREATION 
>   libs/kotext/KoTextAnchor.h 4ec3f03 
>   libs/kotext/KoTextAnchor.cpp 1f2e8c5 
>   libs/kotext/KoTextEditor.h 10c59a6 
>   libs/kotext/KoTextEditor.cpp 1929e07 
>   libs/kotext/KoTextRange.h 78515d9 
>   libs/kotext/KoTextRange.cpp 61cbfa2 
>   libs/kotext/commands/ChangeAnchorPropertiesCommand.h f1448ad 
>   libs/kotext/commands/ChangeAnchorPropertiesCommand.cpp 50e9f55 
>   libs/kotext/commands/ChangeTrackedDeleteCommand.cpp 966d990 
>   libs/kotext/commands/DeleteAnchorsCommand.h d98b858 
>   libs/kotext/commands/DeleteAnchorsCommand.cpp d32f5f1 
>   libs/kotext/commands/DeleteCommand.h a0004a6 
>   libs/kotext/commands/DeleteCommand.cpp 5d39f4f7 
>   libs/kotext/opendocument/KoTextLoader.cpp 29165e1 
>   libs/kotext/opendocument/KoTextSharedLoadingData.h b247992 
>   libs/kotext/opendocument/KoTextSharedLoadingData.cpp ff609d1 
>   libs/kotext/opendocument/KoTextWriter_p.h f3466e1 
>   libs/kotext/opendocument/KoTextWriter_p.cpp 39cf629 
>   libs/textlayout/AnchorStrategy.h f0853f1 
>   libs/textlayout/AnchorStrategy.cpp a7dbec1 
>   libs/textlayout/FloatingAnchorStrategy.h 090efea 
>   libs/textlayout/FloatingAnchorStrategy.cpp 1364df1 
>   libs/textlayout/InlineAnchorStrategy.h 24d3051 
>   libs/textlayout/InlineAnchorStrategy.cpp c21f62b 
>   libs/textlayout/KoTextDocumentLayout.h ef7c197 
>   libs/textlayout/KoTextDocumentLayout.cpp dcb4c8a 
>   libs/textlayout/KoTextLayoutArea.cpp 4f96f19 
>   libs/textlayout/KoTextShapeContainerModel.h 93b8a4d 
>   libs/textlayout/KoTextShapeContainerModel.cpp f19efa5 
>   libs/textlayout/KoTextShapeData.cpp 9d096ef 
>   plugins/textshape/commands/ShowChangesCommand.cpp a486c113 
>   words/part/KWDocument.h dc64e5e 
>   words/part/KWDocument.cpp 67f808d 
>   words/part/KWOdfSharedLoadingData.h 7893cb9 
>   words/part/KWOdfSharedLoadingData.cpp 58f533b 
>   words/part/KWOdfWriter.cpp c9c72f7 
>   words/part/KWRootAreaProvider.cpp 9a86d5d 
>   words/part/KWView.h e86cc5c 
>   words/part/KWView.cpp c9405df 
>   words/part/commands/KWShapeCreateCommand.h a6e2852 
>   words/part/commands/KWShapeCreateCommand.cpp 4781c390 
>   words/part/dialogs/KWAnchoringProperties.h 14ac2d4 
>   words/part/dialogs/KWAnchoringProperties.cpp fe97a23 
>   words/part/frames/KWFrame.h 757c4ec 
>   words/part/frames/KWFrame.cpp e30e8f0 
>   words/part/frames/KWFrameLayout.cpp 5ad85ed 
>   words/part/frames/KWTextFrameSet.cpp 1088541 
> 
> Diff: http://git.reviewboard.kde.org/r/108459/diff/
> 
> 
> Testing
> -------
> 
> I've create two small documents and they load fine.
> 
> I'd like Thorsten to run cstester on this to be sure
> 
> This review is not for final approval - as i need a second step, but it's a nice logical point to do a review
> 
> In several commands i've disabled some stuff because it would be a vaste of time to fix it only to break it again with part2.
> But loading and saving and display should be just as good as before, and the code so far should make sense - so please review that
> 
> 
> Thanks,
> 
> C. Boemann
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20130125/9a332705/attachment.htm>