Review Request: undo,redo of style changes

Pierre Stirnweiss pstirnweiss at googlemail.com
Wed Jan 25 08:16:30 GMT 2012 

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


In general looks good. I have not tried the code yet, will probably be able to do more this evening.


libs/kotext/KoTextDocument.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8285>

    More a question than an issue:
    Shouldn't we have brackets even around one liners?



libs/kotext/commands/ChangeStylesCommand.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8288>

    I am not sure I fully understand the logic/flow of that one.
    Can we have a "commentCode --verbosity=3" instead of "commentCode --verbosity=1"?
    Specifically: Can you add here that instantlyExecuteCommand will ensure that all subsequent internal to the editor's document will get an undo/redo parented to this one? (so there is no need to dig into the KoTextEditor undo/redo stuff.
    Also, can you please explain the flow of the first call and any subsequent calls to redo?



libs/kotext/commands/ChangeStylesMacroCommand.h
<http://git.reviewboard.kde.org/r/103779/#comment8289>

    Can you please explain the rationale behind 2 commands?
    In which case will you need only the ChangeStylesCommand without the Macro one, in which case do we need both?



libs/kotext/commands/ChangeStylesMacroCommand.h
<http://git.reviewboard.kde.org/r/103779/#comment8290>

    Why a QList for the first 5 and a QSet for the last one?



libs/kotext/styles/ChangeFollower.h
<http://git.reviewboard.kde.org/r/103779/#comment8291>

    s/parant/parent



libs/kotext/styles/ChangeFollower.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8292>

    Won't that override that locally modified char formatting?
    We don't necessarily want that:
    - let's say I have a standard style: Sans Serif, 10pt, not bold.
    - I have two words I want to highlight, so I make them locally bold.
    - for legibility I decide that 12pt is better. I fire up the styleManager and change 10pt to 12pt.
    - applying the style like that will apply "not bold" to the whole paragraph, making my 2 words regular again.



libs/kotext/styles/ChangeFollower.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8308>

    do we really need these?
    and then why no kDebugs?



libs/kotext/styles/ChangeFollower.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8309>

    same as above



libs/kotext/styles/KoStyleManager.h
<http://git.reviewboard.kde.org/r/103779/#comment8294>

    Some code comment?



libs/kotext/styles/KoStyleManager.h
<http://git.reviewboard.kde.org/r/103779/#comment8293>

    Unwanted space



libs/kotext/styles/KoStyleManager.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8295>

    Same as before QList/QSet



libs/kotext/styles/KoStyleManager.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8296>

    Do we prefer to crash in production also?
    Shouldn't we handle newStyle=0?



libs/kotext/styles/KoStyleManager.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8298>

    Same as above.



libs/kotext/styles/KoStyleManager.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8307>

    Same comment for one liners



libs/kotext/styles/KoStyleManager.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8300>

    I guess they will also later be handled in a command?
    Ditto for all the others further down.



libs/kotext/styles/KoStyleManager.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8304>

    The diff seems odd here, what happens to updateAlteredStyles()?



libs/kotext/styles/KoStyleManager.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8305>

    The diff seems odd here, what happens to updateAlteredStyles() and requestFireUpdate()?



libs/kotext/styles/KoStyleManager.cpp
<http://git.reviewboard.kde.org/r/103779/#comment8306>

    Is that really necessary?
    Seems odd to me.
    There is an setUndoStack after all.


- Pierre Stirnweiss


On Jan. 24, 2012, 9:52 a.m., C. Boemann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103779/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2012, 9:52 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
>     Implement undo of Style changes
>     
>     Two new methods are introduced: beginEdit() and endEdit() to mark
>     when the editing happens. At endEdit() the collected changes
>     are applied to any attached documents like before.
>     
>     The difference is that it now happens through ChangeStylesMacroCommand
>     Which in turn uses ChangeStylesCommand
>     
>     The role of ChangeStylesMacroCommand is to act as a macro plus to
>     make sure the styles within the stylemanager itself are updated.
>     
>     The role of ChangeStylesCommand is to update the qtextdocuments.
> 
> 
> Diffs
> -----
> 
>   libs/kotext/CMakeLists.txt 6507227 
>   libs/kotext/KoTextDocument.cpp e551825 
>   libs/kotext/commands/ChangeStylesCommand.h PRE-CREATION 
>   libs/kotext/commands/ChangeStylesCommand.cpp PRE-CREATION 
>   libs/kotext/commands/ChangeStylesMacroCommand.h PRE-CREATION 
>   libs/kotext/commands/ChangeStylesMacroCommand.cpp PRE-CREATION 
>   libs/kotext/opendocument/tests/TestChangeTracking.cpp 3aac170 
>   libs/kotext/opendocument/tests/TestLoadStyle.cpp cafe7d9 
>   libs/kotext/opendocument/tests/TestLoading.cpp d464480 
>   libs/kotext/styles/ChangeFollower.h 5aa42b0 
>   libs/kotext/styles/ChangeFollower.cpp 5b10a65 
>   libs/kotext/styles/KoCharacterStyle.cpp 9783bc3 
>   libs/kotext/styles/KoParagraphStyle.cpp b7a5159 
>   libs/kotext/styles/KoStyleManager.h bf6076c 
>   libs/kotext/styles/KoStyleManager.cpp c5f645b 
>   libs/kotext/styles/tests/TestListStyle.cpp 47c1137 
>   libs/kotext/tests/TestKoTextEditor.cpp 751c465 
>   libs/textlayout/tests/TestBlockLayout.cpp bb2cc16 
>   libs/textlayout/tests/TestDocumentLayout.cpp 277178f 
>   libs/textlayout/tests/TestTableLayout.cpp 2bcf0a2 
>   plugins/textshape/dialogs/StyleManager.cpp 2e57137 
>   tables/Map.cpp cd77d3a 
> 
> Diff: http://git.reviewboard.kde.org/r/103779/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> C. Boemann
> 
>

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

More information about the calligra-devel mailing list