Simplify attachment

[dcaliste]

Try to centralise the code responsible of copying attachments into the Downloads directory in EmailAgent only. Also make this a private implementation in EmailAgent. It avoids code duplication and put the attachment related operations in one place only : the agent.

To achieve this:

• attachmentDownloadStatus() from the agent has been extended to also report Downloaded and NotDownloaded status by inquiring the presence of the destination file instead of delegating this to attachmentlistmodel,
• resetModel() from attachmentlistmodel.cpp has been simplified to rely only on the status and the path reported by the agent.

@rainemak , this brings me to ask you why you added a QFileWatcher in attachmentlistmodel with commit edce7bd ? The commit message is a bit unclear about this specific addition. It dealt with the change in downloadAttachment to return a boolean (which is great) and with the fact that the URL is empty if the file has not been saved yet (which is fine also). It seems to me that the EmailAgent::attachmentPathChanged signal would be enough for attachment that are required to be downloaded. I tested the PR here with a POP3 account where the email content is always downloaded and with an IMAP4 one, where I looked at already downloaded attachment, or triggered download on tap. All cases seemed to work.

Additionally, this also reduces the complexity in AttachmentListModel and save roughly 50 lines all in all. @pvuorela, may you give a look into this also when you will have the time to ?

[pvuorela]

This was left halfway reviewed and now has been conflicting too on one place.

Guess it should be mostly ok, left a couple comments. Slightly still a bit unsure how these attachment play along when the saved file can be deleted while the message content remains downloaded, etc.

[pvuorela]

Still needs a rebase

[dcaliste]

Sorry, I noticed the rebase status too late before going to eat...

Slightly still a bit unsure how these attachment play along when the saved file can be deleted while the message content remains downloaded, etc.

If I'm not mistaken, the new code path in that case should be the following:

• the attachmentlistmodel is exposing a NotDownloaded status since the file does not exist and the UI thus propose the file to be doanloaded,
• on download request, the method in EmailAgent notices that the part containing the attachment actually has a body (since the file still exists in the ~/.qmf/mails/ directory,
• the part body is directly saved in the download location and the status changes for Downloaded. The UI is then proposing to open the file.

Actually, this was supposed to be the former code path also, since the changes are about the Download status that is reported by the agent if the file exists, instead of the attachment model monitoring itself the download location on disk.

Which finally raises a question : what happen now if the file is deleted while the email is opened, since there is not disk monitoring anymore... I need to check.

[dcaliste]

Testing on device, it seems that deleting an attachment (for example opening a PDF in the document application, and deleting it there), and coming back to the email application already creates an issue before this PR. Taping again on the attachment in the email view does not open the attachment anymore and raise a notification that the file does not exists. Actually the old FileWatcher object was not looking at file deletion, as far as I can tell in the callback.

Maybe we can have a separated PR for this specific issue ?

[pvuorela]

Yea, not sure if the current situation with transient downloaded state and not following the deletion while in view is any better. What I also tested this seemed mostly comparable so indeed any other changes can be done separately.

Surprisingly tricky this downloaded state, and if changes done, those could be needed jolla-email app too. Thinking if it would be better to consider downloaded (content in qmf), and saved (file created on disk) more separate states. Also would be nice if we didn't need to use that downloaded file structure, perhaps ideally downloaded files would be just put into generic downloads folder.

But if you're ok having this in now(?), I could integrate.

[dcaliste]

Also would be nice if we didn't need to use that downloaded file structure, perhaps ideally downloaded files would be just put into generic downloads folder.

I would love to address this, surely. Having the save path in a unique function in the EmailAgent as proposed in this PR is a first step, I think. We also need to keep the possibility to easily delete all saved files when deleting the email account. So we need to keep the first path level (made up from the account id, or what ever). Or, should we ? When deleting the email account, should we instead just delete the QMF downladed parts and not touch to the saved ones ? For the file themselves, there is no need to add more path hierarchy with the mail id and the part id. Just saving with the provided filename, appending a -%XX number if necessary to avoid collision would be enough. And much simpler to recover where the file is located when navigating with a file browser for instance.

Thinking if it would be better to consider downloaded (content in qmf), and saved (file created on disk) more separate states.

That's a very good suggestion. Indeed, that means to have a little change in jolla-email. I'll give a look in a separated PR for these.

not following the deletion while in view is any better.

This issue is also a separated one. I need to bring back a FileWatcher over the saved files to change the status to NotDownloaded when deleted. I'll create an additional PR for this also since it's not a regression of this current PR.

[pvuorela]

We also need to keep the possibility to easily delete all saved files when deleting the email account. So we need to keep the first path level (made up from the account id, or what ever). Or, should we ? When deleting the email account, should we instead just delete the QMF downladed parts and not touch to the saved ones ?

I think it should be sufficient to delete only the qmf content on account removal. The saved file is requested by the user so in a way it's owned by the user.

For the file themselves, there is no need to add more path hierarchy with the mail id and the part id. Just saving with the provided filename, appending a -%XX number if necessary to avoid collision would be enough.

Tricky part there could be the case when user reopens an email and opens an attachment. If it's already saved as a file (and still exists), that file should be opened and no new one created. Thus probably need to store the saved filename somewhere.

This issue is also a separated one. I need to bring back a FileWatcher over the saved files to change the status to NotDownloaded when deleted. I'll create an additional PR for this also since it's not a regression of this current PR.

Pondering how we'd really need the file watcher. Guess mostly this matters when the attachment is opened at which time the existence could be checked too. Also deleting the file would be still sort of "Downloaded" state if the data is in qmf side.

[dcaliste]

I think it should be sufficient to delete only the qmf content on account removal. The saved file is requested by the user so in a way it's owned by the user.

I totally share this point of view. I'll keep it in mind when touching the saving mechanism. It may require to give a look to the buteo email plugin or to the account components, if the directory under ~/Downloads/mail_attachments/ is deleted or not.

Tricky part there could be the case when user reopens an email and opens an attachment.

Ah, sure, thanks for reminding this use case. I forgot about it. It's a tricky part. Need more thoughts about it.

Pondering how we'd really need the file watcher.

I'm proposing as an additional commit a solution for the attachment deletion issue using a FileWatcher. I agree that it could be done differently by requesting to the EmailAgent the path when using the url getter. What do you prefer :

• remove the additional commit here and accept first this PR without it,
• continue here the discussion of wether it's better to used a FileWatcher, or remove the url cache value from the Attachment structure and recompute it on every access ?

[pvuorela]

Ah, sure, thanks for reminding this use case. I forgot about it. It's a tricky part. Need more thoughts about it.

Was pondering myself something like nemo-email or app specific database for the attachment location -> file mappings. Or if qmf allowed to store some extra metadata then that could be an option.

I'm proposing as an additional commit a solution for the attachment deletion issue using a FileWatcher. I agree that it could be done differently by requesting to the EmailAgent the path when using the url getter. What do you prefer :

For one thing that could be better if it didn't add separate files to the watcher as there are limits how many things can be tracked with inotify. Limit maybe not easily reached, but the app could be running for some time.

But as the file deletion is a different issue, maybe let's just drop it here and integrate the existing things first, shall we.

[dcaliste]

But as the file deletion is a different issue, maybe let's just drop it here and integrate the existing things first, shall we.

Ok, sure. I've removed the additional commit. I'll submit it in a separated PR.