Page MenuHomekolab.org

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.

Details

Reviewers
None

Diff Detail

Branch
dev/mimetreeinterface
Lint
No Linters Available
Unit
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
framework/domain/mimetreeparser/interface.h
59

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

76

parent()

92

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

98

*disposition

99

Isn't this a string?

104

void overrideCharset(const QByteArray &charset) ?

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

138

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?

141–142

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

181

Where is CryptoBackend defined?

209

Where would we find this part?

274

Where's KeyTrust defined?

288

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

314

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

knauss added inline comments.
framework/domain/mimetreeparser/interface.h
92

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

104

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.

138

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);
141–142

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

181

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

209

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

274

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

288

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

314

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
framework/domain/mimetreeparser/test.cpp
43

Same as above?

mollekopf added inline comments.Jul 14 2016, 3:53 PM
framework/domain/mimetreeparser/interface.h
92

In that case please clean it up and map explicitly.

138

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.

141–142

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

181

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

209

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?

274

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

288

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?

314

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
framework/domain/mimetreeparser/interface.h
141–142

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

314

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 {
  public:
     str::shared_ptr<gpgmestructure> getGPGmepointer();
}

and in private header:

class gpgmestructure
{
  public:
    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:
https://phabricator.kde.org/rKUBE0ed9290279f0fa50ece3bed46813fb863a7713eb

We also want to chagne the discussion to phabricator.kde.org