Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nemo-qml-plugin-email] Simplify body creation. #17

Merged
merged 1 commit into from Mar 4, 2024

Conversation

dcaliste
Copy link
Contributor

Adding attachment already move the body
to a first part and set a multipart/mixed
content type, in the QMF code.

@pvuorela, still looking on attachment code, I've noticed that some code were duplicated between QMF and the nemo plugin. But, it's a bit tricky in the sense, that setAttachments() actually changes the multipart to multipart/mixed and clear the existing parts. Which is basically what was done in the plugin. But the clear parts in the plugin was done before setting the body. From my understanding, this should not have any impact. Tests also show no regression (creating a draft with attachment, then reediting the draft to remove the attachment for instance). But I'm not 100% sure because of the slightly different code path and QMF being quite a complex code.

Another question also: I needed to add the change of attachment flag because removing the attachments from a draft didn't remove the icon in the email list (already existing bug). I think, this does not belong to here and should be done at QMF level, since setting the flag is done at QMF level by the call to setAttachments(). But I'm not completely sure where to put it. It seems to me that it should be in ::setBody(). Looking at the code there, I've noticed that you can set a body to a partcontainer that already contains parts (_messageParts is non empty), which is basically what we're doing when removing the attachments of draft with attachments. In that case, the _messageParts is not cleared, and you can still have ::hasBody() that is true and ::partCount() that is strictly greater than 0. Which is inconsistent as far as I can say. Do you agree with me ? Should I provide a patch in QMF that empties the parts when setting a non empty body (and as a consequence unset the attachment flag) ?

@dcaliste
Copy link
Contributor Author

Finally, I convince myself to patch QMF to empty a part we are setting a body to, see https://codereview.qt-project.org/c/qt-labs/messagingframework/+/531966?usp=search

@pvuorela
Copy link
Contributor

So, suppose the clearParts omission is now requiring the messagingframework update to go in, right?

Adding attachment already move the body
to a first part and set a multipart/mixed
content type, in the QMF code.
@dcaliste
Copy link
Contributor Author

Indeed, I'm waiting for the naming scheme patch to go in upstream, and I'll update sailfishos/messagingframework#15 accordingly. Since upstream now contains https://codereview.qt-project.org/c/qt-labs/messagingframework/+/531966, merging sailfishos/messagingframework#15 will allow to accept this PR.

@dcaliste
Copy link
Contributor Author

Finaly, since setBody() is resetting the attachment flag in QMF, I've removed the flag change from this piece of code. With the latest PR from messagingframework, it is now possible to check this PR. Thank you @pvuorela

@pvuorela pvuorela merged commit aeb62a2 into sailfishos:master Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants