Review Request 118532: Proper handling of sections for Words and Author

[Inge Wallin]

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


I have looked through this patch and in general it looks very good.  I found some small language and naming issues which I pointed out.

But I don't know the internals of the text engine well enough to be able to say if the solution is the right way to solve things. So I'll let Camilla give the final ok.


libs/kotext/KoTextEditor_format.cpp
<https://git.reviewboard.kde.org/r/118532/#comment41228>

    Language and naming issue: Are these two really plurals? 



libs/kotext/commands/DeleteCommand.cpp
<https://git.reviewboard.kde.org/r/118532/#comment41230>

    Is there an ownership here?  Unless I misunderstood something a better name would be isEntirelyInsideBlock(), right?



libs/kotext/commands/DeleteCommand.cpp
<https://git.reviewboard.kde.org/r/118532/#comment41232>

    Also language issue: either doesBeginInside or beginsInside. This seems like a minor issue but since begin is a verb you would think about this as an action and not a bool variable.



libs/kotext/commands/DeleteCommand.cpp
<https://git.reviewboard.kde.org/r/118532/#comment41233>

    Why all these comments?  Will this code be removed before the merge or is there some other reason?



libs/kotext/opendocument/KoTextLoader.h
<https://git.reviewboard.kde.org/r/118532/#comment41229>

    I suggest a small enum here instead of a bool.  I saw this issue where you called this function and I had problems understanding it. 



libs/kotext/opendocument/KoTextWriter_p.cpp
<https://git.reviewboard.kde.org/r/118532/#comment41234>

    spelling



libs/kotext/opendocument/KoTextWriter_p.cpp
<https://git.reviewboard.kde.org/r/118532/#comment41235>

    I don't understand this comment.



libs/kotext/opendocument/KoTextWriter_p.cpp
<https://git.reviewboard.kde.org/r/118532/#comment41236>

    same here :)
    


- Inge Wallin


On June 4, 2014, 4:30 p.m., Denis Kuplyakov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118532/
> -----------------------------------------------------------
> 
> (Updated June 4, 2014, 4:30 p.m.)
> 
> 
> Review request for Calligra, Camilla Boemann, Inge Wallin, and Dan Leinir Turthra Jensen.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> This adds proper section handling to code. It concentrates on managing to not break section structure during editing of text.
> Patch fixes many things:
> 1) Insertion of text into sections
> 2) Deletion of text with sections
> 3) Cut, paste of text with sections
> 4) Applying style doesn't remove section info
> 5) WORDS product set added for fast building of Words and Author
> 6) Sections visual indication (need improvements)
> 
> Future TODOS:
> 1) improving section indication
> 2) ability to insert section
> 3) adding unittests
> 
> 
> Diffs
> -----
> 
>   cmake/productsets/words.cmake PRE-CREATION 
>   libs/kotext/KoSection.h ac3fdd8 
>   libs/kotext/KoSection.cpp ca414b0 
>   libs/kotext/KoTextEditor.cpp efc9dd0 
>   libs/kotext/KoTextEditor_format.cpp dacc19a 
>   libs/kotext/KoTextPaste.cpp de89bd8 
>   libs/kotext/commands/DeleteCommand.h f9dd2ef 
>   libs/kotext/commands/DeleteCommand.cpp 5cb92527 
>   libs/kotext/opendocument/KoTextLoader.h ca92573 
>   libs/kotext/opendocument/KoTextLoader.cpp bf3044e 
>   libs/kotext/opendocument/KoTextWriter_p.cpp c03cebb 
>   libs/odf/KoOdfPaste.cpp 1e46546 
>   libs/textlayout/KoTextLayoutArea.h f1b900a 
>   libs/textlayout/KoTextLayoutArea_paint.cpp 2982c60 
>   words/part/CMakeLists.txt 2bf1b0b 
>   words/part/KWFactory.cpp 9894f40 
>   words/part/dockers/KWDebugDocker.h PRE-CREATION 
>   words/part/dockers/KWDebugDocker.cpp PRE-CREATION 
>   words/part/dockers/KWDebugDockerFactory.h PRE-CREATION 
>   words/part/dockers/KWDebugDockerFactory.cpp PRE-CREATION 
>   words/part/dockers/KWDebugWidget.h PRE-CREATION 
>   words/part/dockers/KWDebugWidget.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118532/diff/
> 
> 
> Testing
> -------
> 
> I have tested all functionality with emphasis on deletion. It has the most complicated code and need very good testing.
> You can use LibreOffice Writer to create document with sections and then test editing it in words. And I will attach one I've used.
> 
> 
> File Attachments
> ----------------
> 
> File for testing
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/06/04/f87d4ce2-3ab9-48a4-a880-4caa586790fc__Test-orig.odt
> 
> 
> Thanks,
> 
> Denis Kuplyakov
> 
>

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