Review Request: undo,redo of style changes

[C. Boemann]

> On Jan. 25, 2012, 8:16 a.m., Pierre Stirnweiss wrote:
> > libs/kotext/commands/ChangeStylesCommand.cpp, lines 48-66
> > <http://git.reviewboard.kde.org/r/103779/diff/1/?file=47731#file47731line48>
> >
> >     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?

I've actually written a txt document to tell these things so we don't have to repeat it in all commands. I call it the "macro first pattern"


> On Jan. 25, 2012, 8:16 a.m., Pierre Stirnweiss wrote:
> > libs/kotext/commands/ChangeStylesMacroCommand.h, line 1
> > <http://git.reviewboard.kde.org/r/103779/diff/1/?file=47732#file47732line1>
> >
> >     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?

we can never do without the macro, but it needs to be in two layers as ChangeStylesCommand work on a single qtextdocument and ChangeStylesMacroCommand combines ChangeStylesCommand from several documents into one and also takes care of updating the KoStyleManager itself


> On Jan. 25, 2012, 8:16 a.m., Pierre Stirnweiss wrote:
> > libs/kotext/commands/ChangeStylesMacroCommand.h, lines 53-58
> > <http://git.reviewboard.kde.org/r/103779/diff/1/?file=47732#file47732line53>
> >
> >     Why a QList for the first 5 and a QSet for the last one?

Well a qlist is needed for the first as we need to search through them in a timely manner,

and a qset because it's just faster for looking up an int, than searhing through a list


> On Jan. 25, 2012, 8:16 a.m., Pierre Stirnweiss wrote:
> > libs/kotext/styles/ChangeFollower.cpp, lines 59-71
> > <http://git.reviewboard.kde.org/r/103779/diff/1/?file=47738#file47738line59>
> >
> >     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.

Yes it would, and direct formatting too. So for sure, this code needs to be changed, but that should be a separate change, and i've already discussed it with gopalK because an even bigger issue here is lists shouldn't be overwritten either


> On Jan. 25, 2012, 8:16 a.m., Pierre Stirnweiss wrote:
> > libs/kotext/styles/KoStyleManager.cpp, lines 493-494
> > <http://git.reviewboard.kde.org/r/103779/diff/1/?file=47742#file47742line493>
> >
> >     Do we prefer to crash in production also?
> >     Shouldn't we handle newStyle=0?

I guess


- C.


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


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/27854c90/attachment.htm>