A new libmessageparser interface
Needs ReviewPublic

Authored by mollekopf on Jul 14 2016, 1:21 PM.
This revision needs review, but there are no reviewers specified.



Diff Detail

No Linters Available
No Unit Test Coverage
Build Status
Buildable 4170
Build 4171: arc lint + arc unit

Event Timeline

mollekopf updated this revision to Diff 431.Jul 14 2016, 1:21 PM
mollekopf retitled this revision from to A new libmessageparser interface.
mollekopf updated this object.
mollekopf edited the test plan for this revision. (Show Details)
mollekopf added a project: Kube.
mollekopf added inline comments.Jul 14 2016, 2:32 PM

Consider using a typedef: Part::Ptr for all std::shared_ptr usages




Why do we have it if we don't use it?




Isn't this a string?


void overrideCharset(const QByteArray &charset) ?

I assume the part needs the override charset to be able to correctly return content()?


You can't overload by return type. Perhaps encodedContent()?
Instead of the set/overrideCharset function above, could we just pass the charset to this function or do we need it of other things?


These two should be in MimePart, they can apply to attachments as well.


Where is CryptoBackend defined?


Where would we find this part?


Where's KeyTrust defined?


Since we have a static interface, how would you react to changes?


Can we avoid the GpgME dependency in the interface?
In case we used something else under the hood (like pep).

knauss added inline comments.

just a copy of the KMime:Content enum with these comments :D


overrideCharset is only needed if you really want to overwrite the default charset defined in the mail. The default is not to overwrite the charset.


well the default should be that you don't need to change the charset at all, becuase the sender emailclient is configured correctly :D, thats why I dislike another parameter. But maybe:

QString encodedContent();  //use default charset
QString encodedContent(overwrittenCharset);

muste be moved to Part, because also TextParts can be encrepted/signed


currently it is not configured, inside mimetreeparser it is a pointer to smime/pgp backend of Gpgme


currently only if smimeType == QLatin1String("certs-only")
but i would suggest to also use this type, if we have attached pgpkeys.


Currently nowhere, but to be more flexible, we need a Class here that can be implemented for different KeyTrust Types in their Subcalsses.


Well for QML we anyhow need QObjects with properties, so we also can add signals, the underlaying gpgme is already async.


The problem is that if we make to genric here, we end with a libkleo2 than is only a interface for a interface, that has only one implementation. As I already written if we need notation for mail verification, i'm not sure. And a client may need to show the full details of a signature. And these may be need to displayed differntly for every signature type.

I have no good idea here how to create a nice interface.

mollekopf added inline comments.Jul 14 2016, 3:27 PM

Same as above?

mollekopf added inline comments.Jul 14 2016, 3:53 PM

In that case please clean it up and map explicitly.


Yes, you can either have default argument or do an overload.

My point is if overrideCharset above is only needed for this, then I'd rather just have the parameter on this function.


At which point the MimePart/Part distinction becomes questionable because the only other part is the error part.


If it becomes part of the interface we need to know what it is.


Why would we not just search for an attachment part of a specific mime-type?
Do we need an extra part for that?
What's the benefit?


hmm... We need one consistent behaviour though for the UI, do you already have ideas how that interface could look like?


No, we switched to models a while ago, so there is no requirement for QObjects at all.
Also, it wouldn't help since the change would still have to go through this interface.

The current interface doesn't look like it's asynchronous, so if we really need that feature we need some sort of callback or signal.

Are there other properties that change dynamically?


The alternative is that we directly return qgpgme structures, which means you'll have to stick with that and trust in the binary compatibility promises since you're interface will inherit any problems in that.
If you want to do that do it properly though and directly use the QGpgMe enums etc.

Another variant would be to completely hide anything qgpgme, and you just define everything that we need to display in your interface.
You can still provide i.e. a void pointer that you can cast to the gpgme container based on the backend if you really need to.

knauss added inline comments.Jul 14 2016, 6:37 PM

No we also have TextPart "just" representing a QByteArray. To make GPGInline workable. And other structural elements like EncryptionPart and SignaturePart.

So we have different types of Parts:
StructuralParts: SignaturePart, EncryptionPart, MainContentPart
ContentParts: MimePart, AttachmentPart, TextPart


void pointer - no way with that we really make the software not safger in terms of BIC and API breakage :D Because now nobody knows, that is inside this void pointer.

can something like this help:

class gpgmestructure;

class Signature {
     str::shared_ptr<gpgmestructure> getGPGmepointer();

and in private header:

class gpgmestructure
    GpgMe::Signature signature();

this way we make public interface free of use of GpgMe and make it obvious to use the private headers with risk?

If we have the risk of BIC/API breakage, than we need to add complete interface for the GpgMe structures.

This means:

class Singature

class GPGSignature : public Signaute

and anytime in future maybe:

class PEPSignature : public Signature

mmh this workflow does not work - if I update the branch the diff is not updated and the diff is only show for one commit.

I updated the patch:

We also want to chagne the discussion to