From calligra-devel Fri Feb 24 11:58:16 2012 From: "Thorsten Zachmann" Date: Fri, 24 Feb 2012 11:58:16 +0000 To: calligra-devel Subject: Re: Review Request: Undo redo framework for text refactored Message-Id: <20120224115816.15608.8548 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=calligra-devel&m=133008473701517 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============3453125178110177193==" --===============3453125178110177193== Content-Type: multipart/alternative; boundary="===============2922582111657382586==" --===============2922582111657382586== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104047/#review10863 ----------------------------------------------------------- The works looks quite good. Some comments. I did not test the code yet. libs/flake/commands/KoShapeDeleteCommand.cpp This seems not to be needed please remvoe. libs/kotext/KoTextEditor.cpp This code block is duplicated quite often. I think it makes sense to re= -factor this so it can be reused e.g. = startCommand(i18nc(...)); = The same can be done for endCommand = However this change can also work to a later state libs/kotext/KoTextEditor_undo.cpp there should be a blank after the while words/part/KWDocument.h Is it enough to do that only for Words? words/part/dialogs/KWAnchoringProperties.cpp I think it would make to code more clear when the ChangeAnchorPropertie= sCommand would be moved here words/part/dialogs/KWFrameDialog.cpp Indention is wrong - Thorsten Zachmann On Feb. 23, 2012, 12:10 p.m., C. Boemann wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104047/ > ----------------------------------------------------------- > = > (Updated Feb. 23, 2012, 12:10 p.m.) > = > = > Review request for Calligra. > = > = > Description > ------- > = > We have made changes to the undo framework in kotexteditor to make things= work. > The DeleteCommand has become much more capable as it handles complex sele= ctions and bookmarks now > The KoTextEditor.cpp file has been split out into 3 files > The StyleChange commands and shapeAnchoring commands have been adapted to= the new frame work > = > = > Diffs > ----- > = > libs/flake/KoShapeController.h abc4358 = > libs/flake/commands/KoShapeDeleteCommand.cpp 519f047 = > libs/kotext/CMakeLists.txt 567649b = > libs/kotext/KoTextDocument.h 56f55e6 = > libs/kotext/KoTextDocument.cpp 5608860 = > libs/kotext/KoTextEditor.h 689051b = > libs/kotext/KoTextEditor.cpp 3c141c7 = > libs/kotext/KoTextEditor_format.cpp PRE-CREATION = > libs/kotext/KoTextEditor_p.h 77725f3 = > libs/kotext/KoTextEditor_undo.cpp PRE-CREATION = > libs/kotext/commands/ChangeAnchorPropertiesCommand.h 4206bb8 = > libs/kotext/commands/ChangeAnchorPropertiesCommand.cpp 7d4b912 = > libs/kotext/commands/ChangeStylesCommand.h accfd31 = > libs/kotext/commands/ChangeStylesCommand.cpp b9fa725 = > libs/kotext/commands/ChangeStylesMacroCommand.cpp 34c3822 = > libs/kotext/commands/ChangeTrackedDeleteCommand.cpp 89edde1 = > libs/kotext/commands/DeleteAnchorsCommand.cpp 621829f = > libs/kotext/commands/DeleteCommand.h 89b448a = > libs/kotext/commands/DeleteCommand.cpp 2a4527d = > libs/kotext/commands/TextPasteCommand.cpp 6b9de68 = > libs/kotext/tests/TestKoTextEditor.cpp bd15b3a = > libs/kundo2/kundo2stack.h c0539fe = > libs/kundo2/kundo2stack.cpp 48d6625 = > libs/main/rdf/KoSemanticStylesheet.cpp 7853dd0 = > libs/main/tests/rdf_test.cpp fb57a5e = > plugins/textshape/TextTool.cpp ddfe47f = > plugins/textshape/commands/TextCutCommand.cpp bdb8eea = > words/part/KWDocument.h 0005bc6 = > words/part/KWDocument.cpp 8e00cb6 = > words/part/dialogs/KWAnchoringProperties.cpp c742e26 = > words/part/dialogs/KWFrameDialog.cpp 0fcc0b7 = > words/part/frames/KWTextFrameSet.cpp ebd532f = > = > Diff: http://git.reviewboard.kde.org/r/104047/diff/ > = > = > Testing > ------- > = > We have played around with undo redo in all the cases we knew were failin= g and quite a lot of other tests. > Undo of many things are still not implemented, but that is not really wha= t this is about. It's about the basic undo, and getting the framework right= . Fixing undo of individual stuff can always be done as bug fixes > One issue we have noted is insertion of table, whic we believe is a qt bu= g. We have one last workaround in our minds, but other than that we will ju= st have to wait for a qt fix > = > = > Thanks, > = > C. Boemann > = > --===============2922582111657382586== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/104047/ 

The works =
looks quite good. Some comments. I did not test the code yet.

= =
libs/flake/commands/KoShapeDeleteCommand.cpp (Diff revision 1) 


  

 

  

    
    

    27
    
#include <KDebug>

  


  

    
    

    28
    

  


 





This seems not to be needed please remvoe.

















 =

 =




 

 
  

   
    libs/kotext/KoTextEditor.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   
void KoTextEditor::splitTableCells()


  

 

  

    
    

    1234
    
    bool hasSelection =3D d=
->caret.hasSelection()=
;

  


  

    
    

    1235
    
    if (!hasSelection=
) {

  


  

    
    

    1236
    
        d->updateState(<=
/span>KoTextEditor::Private::Cus=
tom, i18nc("(qtundo-format)", "Insert Endnote"));

  


  

    
    

    1237
    
    }

  


  

    
    

    1238
    
    else {

  


  

    
    

    1239
    
        KUndo2Command=
 *topCommand =3D beginEditBlock=
(i18nc("(qtundo-format)", "Insert Endnote"));

  


  

    
    

    1240
    
        deleteChar(false, topCommand);
<=
/td>
  


  

    
    

    1241
    
        d->caret.=
beginEditBlock();

  


  

    
    

    1242
    
    }

  


  

    
    

    1243
    

  


 





This code block is duplicated quite often. I think it makes sense to=
 re-factor this so it can be reused e.g.

startCommand(i18nc(...));

The same can be done for endCommand

However this change can also work to a later state

















 =

 =




 

 
  

   
    libs/kotext/KoTextEditor_undo.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   

   
void KoTextEditor::addCommand(KUndo2Command *command)


  

 

  

    
    

    252
    
    while(d->commandStack.top() !=3D command) {

  


 





there should be a blank after the while

















 =

 =




 

 
  

   
    words/part/KWDocument.h
    

     (Diff revision 1)

    
   
  

 

 
  


   
private:


  

 

  

    
    

    271
    
    KoShapeController=
 *m_shapeController;

  


 





Is it enough to do that only for Words?

















 =

 =




 

 
  

   
    words/part/dialogs/KWAnchoringProperties.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   
void KWAnchoringProperties::save(KUndo2Command *macro)


  

 

  

    
    

    574
    

  


 





I think it would make to code more clear when the ChangeAnchorProper=
tiesCommand would be moved here

















 =

 =




 

 
  

   
    words/part/dialogs/KWFrameDialog.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   
KWFrameDialog::~KWFrameDialog()


  

 

  

    
    

    93
    
           KUndo2Command::redo();

  


 





Indention is wrong









- Thorsten






On February 23rd, 2012, 12:10 p.m., C. Boemann wrote:









 

  


Review request for Calligra.


By C. Boemann.




Updated Feb. 23, 2012, 12:10 p.m.








Descripti=
on 

 

 

  
   
We have made changes to the undo framework in kotexteditor t=
o make things work.
The DeleteCommand has become much more capable as it handles complex select=
ions and bookmarks now
The KoTextEditor.cpp file has been split out into 3 files
The StyleChange commands and shapeAnchoring commands have been adapted to t=
he new frame work

  		
 






Testing <=
/h1>


 

  
   
We have played around with undo redo in all the cases we kne=
w were failing and quite a lot of other tests.
Undo of many things are still not implemented, but that is not really what =
this is about. It's about the basic undo, and getting the framework rig=
ht. Fixing undo of individual stuff can always be done as bug fixes
One issue we have noted is insertion of table, whic we believe is a qt bug.=
 We have one last workaround in our minds, but other than that we will just=
 have to wait for a qt fix

  		
 








Diffs=
 


    

 
  • libs/flake/KoShapeController.h (abc4358)
    • 

 
  • libs/flake/commands/KoShapeDeleteCommand.cpp (519f047)
    • 

 
  • libs/kotext/CMakeLists.txt (567649b)
    • 

 
  • libs/kotext/KoTextDocument.h (56f55e6)
    • 

 
  • libs/kotext/KoTextDocument.cpp (5608860)
    • 

 
  • libs/kotext/KoTextEditor.h (689051b)
    • 

 
  • libs/kotext/KoTextEditor.cpp (3c141c7)
    • 

 
  • libs/kotext/KoTextEditor_format.cpp (PRE-C=
REATION)
    • 

 
  • libs/kotext/KoTextEditor_p.h (77725f3)
    • 

 
  • libs/kotext/KoTextEditor_undo.cpp (PRE-CRE=
ATION)
    • 

 
  • libs/kotext/commands/ChangeAnchorPropertiesCommand.h (4206bb8)
    • 

 
  • libs/kotext/commands/ChangeAnchorPropertiesCommand.cpp (7d4b912)
    • 

 
  • libs/kotext/commands/ChangeStylesCommand.h (accfd31)
    • 

 
  • libs/kotext/commands/ChangeStylesCommand.cpp (b9fa725)
    • 

 
  • libs/kotext/commands/ChangeStylesMacroCommand.cpp (34c3822)
    • 

 
  • libs/kotext/commands/ChangeTrackedDeleteCommand.cpp (89edde1)
    • 

 
  • libs/kotext/commands/DeleteAnchorsCommand.cpp (621829f)
    • 

 
  • libs/kotext/commands/DeleteCommand.h (89b4=
48a)
    • 

 
  • libs/kotext/commands/DeleteCommand.cpp (2a=
4527d)
    • 

 
  • libs/kotext/commands/TextPasteCommand.cpp =
(6b9de68)
    • 

 
  • libs/kotext/tests/TestKoTextEditor.cpp (bd=
15b3a)
    • 

 
  • libs/kundo2/kundo2stack.h (c0539fe)=

    • 

 
  • libs/kundo2/kundo2stack.cpp (48d6625)
    • 

 
  • libs/main/rdf/KoSemanticStylesheet.cpp (78=
53dd0)
    • 

 
  • libs/main/tests/rdf_test.cpp (fb57a5e)
    • 

 
  • plugins/textshape/TextTool.cpp (ddfe47f)
    • 

 
  • plugins/textshape/commands/TextCutCommand.cpp (bdb8eea)
    • 

 
  • words/part/KWDocument.h (0005bc6)

 
  • words/part/KWDocument.cpp (8e00cb6)=

    • 

 
  • words/part/dialogs/KWAnchoringProperties.cpp (c742e26)
    • 

 
  • words/part/dialogs/KWFrameDialog.cpp (0fcc=
0b7)
    • 

 
  • words/part/frames/KWTextFrameSet.cpp (ebd5=
32f)
    • 





View Diff





  		
 











  

 



--===============2922582111657382586==--

--===============3453125178110177193==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
calligra-devel mailing list
calligra-devel@kde.org
https://mail.kde.org/mailman/listinfo/calligra-devel

--===============3453125178110177193==--