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

Re-Adding "Send by Email" #1822

Merged
merged 19 commits into from
Nov 2, 2021
Merged

Re-Adding "Send by Email" #1822

merged 19 commits into from
Nov 2, 2021

Conversation

marbetschar
Copy link
Member

@marbetschar marbetschar commented Sep 1, 2021

Using the org.freedesktop.portal.Email portal as suggested by @Marukesu here: elementary/mail#674.

Unfortunately I can't get this to work. The context menu item is shown and I'm able to click on it - but the portal itself is never shown. What am I missing?

@jeremypw
Copy link
Collaborator

jeremypw commented Sep 1, 2021

Just testing this: At the moment it is failing because the file_descriptors are not being created (returning "-1"). dbus-monitor shows that the method call is being made and the contents otherwise look OK.

@jeremypw
Copy link
Collaborator

jeremypw commented Sep 1, 2021

This is what I get from dbus-monitor now:

method call time=1630519639.938736 sender=:1.596 -> destination=org.freedesktop.portal.Desktop serial=142 path=/org/freedesktop/portal/desktop; interface=org.freedesktop.portal.Email; member=ComposeEmail
   string "x11:38005c2"
   array [
      dict entry(
         string "handle_token"
         variant             string "io_elementary_files_1493378944"
      )
      dict entry(
         string "attachment_fds"
         variant             array [
               int32 28
            ]
      )
   ]

However, the email program is not launched nor is any error or response given from the portal. Not sure how to check whether the portal itself is actually working ...

Pasting a "mailto" URI into Firefox does launch the email program but I am not sure whether FireFox is using the portal.

@Marukesu
Copy link
Contributor

Marukesu commented Sep 2, 2021

So, there's two issues:

One is that the portal segfault when there's no address on the options dict, looking at the portal code, it seems that the portal needs at least one address (see flatpak/xdg-desktop-portal-gtk#343).

The second is that vala only support sending a single file descriptor in a DBus method (see GNOME/vala#338). So we need to use the underlying DBusProxy.call_with_unix_fd_list. Looking at the code on libportal, we need to add the file descriptor to the UnixFdList and the index returned by UnixFdList.append to the attachment_fds in options.

you can run /usr/libexec/xdg-desktop-portal-gtk -vr to check if the mailto URI is sending the attachments correctly.

@jeremypw
Copy link
Collaborator

jeremypw commented Sep 2, 2021

@Marukesu Thanks for the diagnosis. According to https://flatpak.github.io/xdg-desktop-portal/portal-docs.html#gdbus-org.freedesktop.portal.Email "All the keys in the options are are optional". Is that not the case if an address is required?

@Marukesu
Copy link
Contributor

Marukesu commented Sep 2, 2021

I would say it's a bug on the GTK backend of the portal. if it was really required the frontend would throw a error.

edit: seems that passing new Variant ("as", null) as the addresses key works too. so it's another alternative for now.

@marbetschar
Copy link
Member Author

The second is that vala only support sending a single file descriptor in a DBus method (see GNOME/vala#338). So we need to use the underlying DBusProxy.call_with_unix_fd_list. Looking at the code on libportal, we need to add the file descriptor to the UnixFdList and the index returned by UnixFdList.append to the attachment_fds in options.

@Marukesu can you please provide some more info how exactly this would be implemented in Vala? I'm not too familiar with DBus and file descriptors yet, so I can't follow unfortunately.

Copy link
Contributor

@Marukesu Marukesu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played a bit with the code, with the suggestions, the portal generates a valid mailto uri with the attachments as expected.

However, i don't see the attachments on mail composer, and it failed to send with mail_backend_session_send_email: assertion 'from != NULL' failed, but that are issues in mail.

plugins/send-by-email/Portal.vala Outdated Show resolved Hide resolved
plugins/send-by-email/Portal.vala Show resolved Hide resolved
plugins/send-by-email/Portal.vala Outdated Show resolved Hide resolved
plugins/send-by-email/plugin.vala Outdated Show resolved Hide resolved
plugins/send-by-email/plugin.vala Outdated Show resolved Hide resolved
plugins/send-by-email/plugin.vala Outdated Show resolved Hide resolved
@marbetschar
Copy link
Member Author

@Marukesu thank you so much for breaking this down - that helped tremendously! Thinks are working now on my end as expected together with elementary/mail#682.

So I'll guess we are ready for a review here :)

@marbetschar marbetschar marked this pull request as ready for review September 7, 2021 06:14
@marbetschar marbetschar requested a review from a team September 20, 2021 08:09
@jeremypw
Copy link
Collaborator

I can confim that this now brings up the Mail program with a new mail with the selected files attached as expected 🎉

I cannot fully confirm that the mail gets sent as expected since I am having trouble setting up Mail to access my GMail account (even with 2FA off and less secure app access enabled). Is there an idiot's guide for doing this available?

Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor code style nitpicks. Otherwise seems to work as far as I can test it.

plugins/send-by-email/plugin.vala Show resolved Hide resolved
plugins/send-by-email/plugin.vala Outdated Show resolved Hide resolved
plugins/send-by-email/plugin.vala Outdated Show resolved Hide resolved
plugins/send-by-email/plugin.vala Show resolved Hide resolved
@marbetschar
Copy link
Member Author

@jeremypw I'm able to add Gmail with the following configuration:

  1. Go to your webmail settings into the tab Forwarding & POP/IMAP: https://mail.google.com/mail/u/0/#settings/fwdandpop
  2. Enable IMAP there
  3. Enable "Less Secure Apps" here: https://support.google.com/accounts/answer/6010255
  4. Add your Gmail account with the default settings via Online Accounts

At least on my end this procedure works w/o any issue.

@jeremypw
Copy link
Collaborator

jeremypw commented Sep 21, 2021

@marbetschar Thanks, that sounds pretty much like what I was was doing but I'll try again. I am trying to add my elementaryos.org account on Odin stable.

@marbetschar
Copy link
Member Author

@jeremypw oh, ok then you might need elementary/switchboard-plug-onlineaccounts#225 by installing Online Accounts from current master. Not sure though, lost a bit track which change exactly fixed what :)

@jeremypw
Copy link
Collaborator

jeremypw commented Oct 5, 2021

@marbetschar I have now setup access to my GMail account using the latest code of the Online Accounts plugin and Mail. This PR successfully produces an email with the selected file attached but after filling in the address, subject and body and attempt to send it resulted in the following message:

Unable to retrieve source for mail submission.

I could successfully send emails (with attachments) from Mail itself.

@Marukesu
Copy link
Contributor

Marukesu commented Oct 5, 2021

@jeremypw When the dialog opens did the sender combobox is setted and active? when mail is closed, if i use this PR, it's empty and inactive, but if mail is already open, the sender is setted, and the combobox is active.

@marbetschar
Copy link
Member Author

@jeremypw this is fixed now in latest master of elementary Mail.

@marbetschar marbetschar reopened this Oct 13, 2021
@jeremypw
Copy link
Collaborator

@marbetschar This now works better but with the limitation that the Mail window must be closed. Trying to send a text file while Mail was open did not show a dialog box (the Mail window just focused). Presumably this is an issue with Mail?

@Marukesu
Copy link
Contributor

Trying to send a text file while Mail was open did not show a dialog box (the Mail window just focused). Presumably this is an issue with Mail?

Oh, it's because mail is now waiting for a signal that is only emitted on the first launch, i totally missed that when reviewing the PR, i'm sorry 😞

@marbetschar
Copy link
Member Author

@jeremypw mea culpa! I was so focused to get this fixed that I totally missed to test this scenario as well 😞

@marbetschar marbetschar requested a review from jeremypw October 25, 2021 17:54
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now works as expected provided Mail is natively installed. However if Mail is flatpak'd then the attachment has 0 bytes and no name. This is not necessarily blocking while Mail is supplied as a deb package but must be fixed before we switch to supplying Flatpak.

@Marukesu
Copy link
Contributor

However if Mail is flatpak'd then the attachment has 0 bytes and no name.

That's because Mail don't ask for any filesystem access other than \tmp\io.elementary.mail and the portal don't export the attachment to the documents portal when needed. I opened a issue in the xdg-desktop-portal-gtk repo about that.

@jeremypw
Copy link
Collaborator

I am I right that Mail is currently being supplied as a deb? There seems to be a deb-packaging branch for version 6.3.0. If that is the case the problem is not blocking.

@marbetschar
Copy link
Member Author

@jeremypw yes, that's correct. Mail is distributed as deb for the time beeing because of elementary/mail#591

@jeremypw
Copy link
Collaborator

@marbetschar OK, thanks. As a stable release of Files has just been made I think it safe enough to merge this now.

@jeremypw jeremypw enabled auto-merge (squash) October 31, 2021 17:48
Copy link
Collaborator

@jeremypw jeremypw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected provided Mail is not Flatpak'd.

@jeremypw jeremypw merged commit a3779bd into master Nov 2, 2021
@jeremypw jeremypw deleted the send-by-email branch November 2, 2021 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants