Make KMime::Content available properly in MessagePart
ClosedPublic

Authored by vkrause on Sep 30 2017, 12:33 PM.

Details

Summary

Most MessagePart sub-classes were doing this themselves so far, but we
need this generically, e.g. to give render plugins access to header data.

Diff Detail

Repository
R94 PIM: Message Library
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
vkrause created this revision.Sep 30 2017, 12:33 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptSep 30 2017, 12:33 PM
Restricted Application added a subscriber: KDE PIM. · View Herald Transcript
knauss added a comment.EditedSep 30 2017, 1:00 PM

The main idea is that the rendering process should not get access to KMime::Content and all calls to this are going via the MessagePart. That is leaks into the renderer is because of getting a fast working solution.
So the right way IMO is to move the getter into proctected and than every MP that needs to have it public mark the getter public or even provide these information the Render needs getFileName, contentDescription ...
But maybe this makes it more completcated than easier.
I know, that my aproch treats plugins autors to writer a propper MessagePart...

so i had in mind something like:

Kmime::Content <-> messagepart <->Renderer

and NO:

Kime:Content <-> Render calls

The problem with that is that we end up duplicating all kinds of KMime API without any gain. MessagePart makes sense for everything that needs decoding/decryption etc, and for changing the hierarchy, but IMHO not for verbatim forwarding of KMime data.
For my use-case (generic highlighting plugin) I need to have access to Content-* headers to check if it's something I want to handle or not.

The problem with that is that we end up duplicating all kinds of KMime API without any gain. MessagePart makes sense for everything that needs decoding/decryption etc, and for changing the hierarchy, but IMHO not for verbatim forwarding of KMime data.
For my use-case (generic highlighting plugin) I need to have access to Content-* headers to check if it's something I want to handle or not.

But this is exactly the usecase for an own MessagePart type. You process a mimetype and create SytaxHighlightMessagePart (SHMP) if you want to handle this part, the MessagePart has access to the KMime::Content and than you handle your specific MP and be happy and i think your SHMP only need the type of the KMime::Content.

My idea is that a plugin should early decide if it wants to handle a mimetype. And not that the process are done and than later on the render decide own we process the mimetype complety different

But that would tie processing and rendering always together to the same plugin, wouldn't it? In which case the entire exercise of splitting this up becomes kinda pointless, we could just render with the same plugin that managed to succeed in processing.

Also, look at it from the plugin developer POV: In this scenario I need to implement process() to decide if I want to deal with it (fair enough), implement my own sub-class of TextMessagePart or AttachmentMessagePart (without any extra data, everything I need is already there, so that's just busy work), fill it with the same logic that TextPlainBodyPartFormatter has presumably (that's highly error prone, that code deals with sign/encrypt state for example), and then I eventually get that passed to my render() method that actually does what my plugin wants to do.
As most plugins are purely dealing with rendering, I think this deserves a more convenient API. Ie. leave processing to things like crypto that actually manipulates the hierarchy, and to do things like determining what's the main body text etc, and allow rendering to work on top of that result.

But that would tie processing and rendering always together to the same plugin, wouldn't it? In which case the entire exercise of splitting this up becomes kinda pointless, we could just render with the same plugin that managed to succeed in processing.

I think in most cases they are connected yes: for vcard, WKS, calendar there is for sure things, that should be done in Processing and not at render step.

Also, look at it from the plugin developer POV: In this scenario I need to implement process() to decide if I want to deal with it (fair enough), implement my own sub-class of TextMessagePart or AttachmentMessagePart (without any extra data, everything I need is already there, so that's just busy work), fill it with the same logic that TextPlainBodyPartFormatter has presumably (that's highly error prone, that code deals with sign/encrypt state for example), and then I eventually get that passed to my render() method that actually does what my plugin wants to do.

well that is a valid point, the formatter need to get much easier! btw. the distintion between AttachmentMessagePart/textmessagepart is also something that really should be removed, the descition if what is an attachment should be done later in ther rendering step.

As most plugins are purely dealing with rendering, I think this deserves a more convenient API. Ie. leave processing to things like crypto that actually manipulates the hierarchy, and to do things like determining what's the main body text etc, and allow rendering to work on top of that result.

Okay we don't come to a good conclusion...

So be pramatic: So far we don't have that many plugins, and these need at least be updated to the recent interface. And this patch helps to do the work.

We can later still make the getter as deprecated and rework, if we have more clearer picture with the plugins...

knauss added inline comments.Sep 30 2017, 1:59 PM
mimetreeparser/src/viewer/objecttreeparser.cpp
224

mp can still be a nullptr, so keep the if statement

vkrause added inline comments.Sep 30 2017, 2:29 PM
mimetreeparser/src/viewer/objecttreeparser.cpp
224

It's asserted to be not null after the return, and with the mimetype fallback handling it indeed can't happen anymore, the generic octet-stream attachment processor will always produce something.

knauss accepted this revision.Sep 30 2017, 3:33 PM
knauss added inline comments.
mimetreeparser/src/viewer/objecttreeparser.cpp
224

okay sounds logic

side question: what happens if i register a plugin for application/octet-stream, that whants to display with AsIcon? than you end up in a endless loop, or not?

This revision is now accepted and ready to land.Sep 30 2017, 3:33 PM
vkrause added inline comments.Sep 30 2017, 6:54 PM
mimetreeparser/src/viewer/objecttreeparser.cpp
224

Right, that would be possible. I think the correct solution would be to replace this entire block with a "continue", we'll then eventually get to the fallback handler and get the icon attachment display that was requested. Makes sense? Then I'll do that in a separate commit.

knauss added inline comments.Oct 1 2017, 12:43 AM
mimetreeparser/src/viewer/objecttreeparser.cpp
224

no we can't do simply "contiune" because than you can't create plugins for "application/octet-stream". But we only need a short time solution until the LegacyMesaagePart is gone. A qcWaning or qcError if mimetype is "application/octet-stream" ? than at least you see the problem...

vkrause added inline comments.Oct 1 2017, 8:24 AM
mimetreeparser/src/viewer/objecttreeparser.cpp
224

An application/octet-stream plugin that would actually render content would not return "AsIcon". "AsIcon" means "render as default attachment icon" here, and that's the same "continue" would give us. IOW "AsIcon" and "Failed" are essentially the same here now, meaning "I can't or don't want to render this", and "Failed" is already handled by "continue".

And any new plugin that correctly implements process() would not end up here anyway, this is just for the 5 existing legacy plugins.

This revision was automatically updated to reflect the committed changes.