Split parsing and rendering of tempNode of encrypted/signed parts.
ClosedPublic

Authored by knauss on Oct 8 2015, 10:49 AM.

Details

Reviewers
vkrause
Group Reviewers
KDE PIM
Maniphest Tasks
T719: Refactor objecttreeparser
Summary

project_reviewers: KDE_PIM

Diff Detail

Lint
No Linters Available
Unit
No Unit Test Coverage
knauss updated this revision to Diff 949.Oct 8 2015, 10:49 AM
knauss retitled this revision from to Split parsing and rendering of tempNode of encrypted/signed parts..
knauss added a reviewer: vkrause.
knauss updated this object.
knauss added a project: KDE PIM.
vkrause added inline comments.Oct 9 2015, 4:14 PM
messageviewer/src/htmlwriter/queuehtmlwriter.h
31

Copyright suggests this is entirely new code, the obsolete Trolltech exception suggests the opposite.

If it's new code I'd suggest to use an up to date GPLv2+ or LGPLv2+ header. If it's based on code with actually that license, please amend the copyright header.

48

At least a one line class documentation might be a good idea ;-)

63

"replay" I assume?

67

This would make Marc cry ;-)

Even using a fairly primitive struct in that vector would be a lot more efficient:

struct Command {
    enum { Begin, End, Reset, ... } type;
    QString s;
    QByteArray b;
}; // 24 byte

That is hardly larger than a single variant, not even considering the variant list around them, and avoiding the extra allocations due to that. And you can replace the string comparisons in replay() with a simple switch/case block.

There are ways to make this even more compact if needed, for example by splitting out the QString/QByteArray members to separate vectors and just keeping the indexes in the struct:

struct Command {
    enum {...} type: 3;
    uint bIndex : 29;
    uint sIndex;
}; // 8 byte, limit of 2^29 embedded part commands and 2^32 commands taking strings, which is probably sufficient.

In any case, both these solutions reduce allocations to just the growing vector, rather than one or two per command, which is far more important than the total memory consumption I think.

messageviewer/src/viewer/messagepart.cpp
345

Hm, this goes against the memory-saving streaming idea of the writer, is there no way we can avoid the record/replay approach? Ie. would it be possible to defer HTML generation to this point?

All other issues are clear to me and I'll update the patch already.

messageviewer/src/viewer/messagepart.cpp
345

At the moment no, our problem at the moment is, that

mSubOtp->parseObjectTreeInternal(textNode); (line 208)

directly creates html output and do not process first and than creates html afterwards. If everything in otp is rewritten with a clean first process than render, we can get rid of this and the QueueHTMLWriter again.

Actually this is one of the main reasons I want to rewrite otp - with this QueueHTMLWriter we are now in the position, that i can split the process part from the redering part.

vkrause edited edge metadata.Oct 11 2015, 9:13 AM

Ah, if it's just a migration aid then that's perfectly fine of course :)

knauss updated this revision to Diff 964.Oct 12 2015, 8:31 AM
knauss marked 6 inline comments as done.
knauss edited edge metadata.

resolved issues from vkrause

vkrause accepted this revision.Oct 12 2015, 4:16 PM
vkrause edited edge metadata.
This revision is now accepted and ready to land.Oct 12 2015, 4:16 PM
knauss closed this revision.Oct 12 2015, 5:20 PM