On Mar 5, 2007, at 8:07 PM, Charles Connell wrote: > On Monday 05 March 2007 05:17, Martijn Klingens wrote: >> On Sunday 04 March 2007 19:53, Charles Connell wrote: >>> I've got a patch here which reimplements the body of a >>> Kopete::Message as >>> a QTextDocument (before it was a QString). >> >> Just some notes on the performance bits, someone else should >> comment on the >> rest ;) >> >>> 4. The infamous unescape() function is no longer necessary, >>> although it >>> is left in as a convenience function. escape() is no longer used >>> as well. >> >> What you didn't properly port is the isRightToLeft() method -- you >> removed >> the code that sets d->isRightToLeft. Either remove the function >> entirely >> and change the call to it into the equivalent of >> >> plainBody().isRightToLeft(); // a QString method >> >> or readd the functionality. > I have added the line: > d->isRightToLeft = d->body->toPlainText().isRightToLeft(); > >> >>> 2. QTextDocument cloning. QTextDocument's inherit QObject, so >>> they have >>> parents. Since Kopete::Message is not based on QObject, I have >>> added a >>> QObject for the QTextDocument to have as a parent. Every >>> QTextDocument* >>> received is cloned so that it has the parent we want it to have. >>> I wish I >>> could just reparent(), but only a QWidget can do that. >> >> Why not a null parent? Parent is not mandatory AFAICS. You just >> need to >> manually delete in the destructor if you do that. >> > I have done just that. I had had a separate parent because I > figured that if > at a later time something else was to use the same > bogus-parent-and-needs-to-be-deleted-in-destructor setup, I could > use the > parent I had set up to make things a little simpler. However, I > seem to have > forgotten to delete d->bodyParent anyways (I knew I had to, but I > redid my > changes a few times and I must have forgotten about it). Anyways, yes, > d->body is now deleted in the destructor of Kopete::Message > >>> Known problems: >>> 1. Speed? Unescape was also the bottleneck for performance, and >>> I'm not >>> sure if QTextDocument's equivalent functions are any better (or >>> worse) >> >> I would still like to see a way to avoid unescape entirely. The >> version of >> unescape that is performance-sensitive is used only for calling >> isRightToLeft() and it doesn't feel well to do a complete >> richtext-to-plaintext conversion just to figure out whether the >> thing is in >> western left-to-right or Hebrew/Arabic RTL. >> >> There must be a way to figure that out without expensive >> conversions... > I believe that unescape is never called in my code, only toPlainText > (). I'm > sincerely hoping that that isn't as expensive, because I use it in > plainBody() obviously, and I know that that is called in many places > throughout Kopete. > > > Anyways, I'm back with a patch for the issues you raised. I would > be happy to > hear more from anyone about it. This is my first experience with an > open > source development team, and also with the kopete source code, so > I'm bound > to mess a few things up. > > Also, just a note: the attached diff is the diff between SVN trunk > and my > development copy. It is not a diff to my previous patch's merged > code or > anything funky like that. It may contain some entries I didn't > cause because > my development code is a few weeks old, and someone may have > changed the SVN > version of kopetemessage.cpp in that time. >
Don't move QPointer out of the .cpp into the header file. It's not needed. We try to limit the number of includes in .h files in order to speed up compile times. Remove the include for QTextDocument from the .h file and move it to the .cpp file. Forward declare QTextDocument instead. doSetBody needs to be made private (if it's not already) and needs to be marked as internal for Doxygen. Add "@internal" to the documentation for that function. The third hunk of kopetemessage.cpp (hunks are identified by the @ signs with line numbers, for example "@@ -91,1 +95,1 @@") is messed up. It looks like you removed the assignment of the new QTextDocument to the body variable. Decide which constructors to keep. We either need the QString versions or the QTextDocument versions, but not both. In doSetBody, you don't check to verify that the QTextDocument already exists. If it does exist, then allocating a new one will cause a memory leak. We need to verify that escapedBody does the same thing it did before this change and that the output is the same. Please write a unit test for that. Pointers for next time: Don't diff each file individually. It makes it harder to review. General questions: Can we use QTextDocumentFragment or whatever it's called for the Message instead of a QTextDocument? My idea was that the messaging session would be represented by a QTextDocument and then we just add fragments to the chat session document and have it go render it. I don't know if it'll work or not, or if it's the best way. It's just my idea. Does this patch stand on it's own? IOW, can we commit it and not mess anything else up? Thanks for your work on this. -- Matt _______________________________________________ kopete-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/kopete-devel
