[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: questions regarding QTextDocument behaviour
From:       Thomas Zander <zander () kde ! org>
Date:       2008-12-05 15:58:13
Message-ID: 200812051658.13327.zander () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Sunday 30. November 2008 20:32:05 Pierre Stirnweiss wrote:
> I have committed a working version of the above. 

I have some observations;

* please follow the KDE coding style. I think having consistent indentation is 
mandatory, so no tabs and 4 spaces. And naturally indent properly to the 
level you are writing on.
Having one consistent coding style everywhere makes it very easy for new 
people to come on board by making things much more readable for all.

* The moving of the code from the selectionHandler to the tool means 
applications can no longer insert a paragraph seperator; only the user can 
via the keyboard. I think we should move that code back.
Imagine KPresenter having a requirement to create some text as one of the 
default sheets. Or having a kross script manipulate text.

* The insertParagraph command doesn't follow the command design pattern; there 
should be no actual data change done in the constructor, only in the redo() 
and undo() methods.  
This is because the 'addCommand' calls redo() on that command.

This combined with the previous point I think we should 
retire this command totally as it worked just fine, just we had some bugs in 
the tool we have to fix anyway.

* The TextInsertTextCommand::mergeWith() implementation is curious; it looks a 
bit complicated due to the amount of static_cast<>s.
A bigger issue I have with it is that for each keystroke we new have a new 
TextInsertTextCommand and a new UndoTextCommand which are then immediately 
deleted again followed by another new UndoTextCommand.

That sounds like a big slowdown (doing a new is very slow due to it using 
malloc).
I think we should merge the functionality of the TextInsertTextCommand into 
the texttool again since its effectively just 10 lines of code.  This also 
makes things less complex by not introducing yet another concept. AFAICT the 
text tool knowing about a current undo command is perfectly adequate for all 
usages we have.

> If this solution is acceptable for everybody, I have already most of the
> editing actions ready.

I'm confused what this means; why would we need more actions or commands? Qt 
has all of them already in the QTexDocument.
Having commands in the texttool doesn't seem to buy us anything. Does it?
Can you please explain your idea behind this?

Would you be able to take this feedback and prepare a patch for this? Please 
post that patch to the list for review first :)

THANK YOU, and keep up the work!
-- 
Thomas Zander

["signature.asc" (application/pgp-signature)]

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


[prev in list] [next in list] [prev in thread] [next in thread]