[ios] Add default email client support to about menu #7862

kirylkaveryn · 2024-04-08T00:32:14Z

v-lozko commented

2024-04-08 00:32:14 +00:00

(Migrated from github.com)

These changes are to add default email client support in the about menu. Issue #7758.

I tested it on my ipad 11 running 17.0.3

I would post a video but opening the email app does share some personal info

These changes are to add default email client support in the about menu. Issue #7758. I tested it on my ipad 11 running 17.0.3 I would post a video but opening the email app does share some personal info

🎉 1

biodranik (Migrated from github.com) reviewed 2024-04-08 07:31:01 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! Did you test it with other mail clients? GMail, Proton for example?

@kirylkaveryn PTAL

Thanks! Did you test it with other mail clients? GMail, Proton for example? @kirylkaveryn PTAL

kirylkaveryn requested changes 2024-04-08 08:43:34 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

					
				@ -484,0 +488,4 @@

				        URLQueryItem(name: "subject", value: subject),

				        URLQueryItem(name: "body", value: body.replacingOccurrences(of: "\n", with: "\r\n")),

				      ]

kirylkaveryn commented

2024-04-08 08:41:29 +00:00

This code doesn't work for me... I'm using Spark (to field):

The Problem is in the toRecipients - it is an array of Strings. And to work as a subject it should be joined using ;.
Please take a look at the code used before for opening other apps:

    func openGmail(subject: String, body: String, recipients: [String]) -> Bool {
      var components = URLComponents(string: "googlegmail://co")!
      components.queryItems = [
        URLQueryItem(name: "to", value: recipients.joined(separator: ";")),
        URLQueryItem(name: "subject", value: subject),
        URLQueryItem(name: "body", value: body),
      ]

      if let url = components.url, UIApplication.shared.canOpenURL(url) {
        UIApplication.shared.open(url)
        return true
      }
      return false
    }

So you can create the same method and handle the default app using URLComponents.
Here is the hardcoded one that I've made just as a reference (needs to be refactored):

func openDefaultApp(subject: String, body: String, recipients: [String]) -> Bool {
    var components = URLComponents(string: "mailto:\(recipients.joined(separator: ";").addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? "")")!
    components.queryItems = [
      URLQueryItem(name: "subject", value: subject),
      URLQueryItem(name: "body", value: body),
    ]

    if let url = components.url, UIApplication.shared.canOpenURL(url) {
      UIApplication.shared.open(url)
      return true
    }
    return false
  }

As a result:

This code doesn't work for me... I'm using Spark (`to` field): <img width="300" alt="image" src="https://github.com/organicmaps/organicmaps/assets/79797627/8db4a210-3838-45fd-bc4b-95178a8c836e"> The Problem is in the `toRecipients` - it is an array of Strings. And to work as a subject it should be joined using `;`. Please take a look at the code used before for opening other apps: ```swift func openGmail(subject: String, body: String, recipients: [String]) -> Bool { var components = URLComponents(string: "googlegmail://co")! components.queryItems = [ URLQueryItem(name: "to", value: recipients.joined(separator: ";")), URLQueryItem(name: "subject", value: subject), URLQueryItem(name: "body", value: body), ] if let url = components.url, UIApplication.shared.canOpenURL(url) { UIApplication.shared.open(url) return true } return false } ``` So you can create the same method and handle the default app using URLComponents. Here is the hardcoded one that I've made just as a reference (needs to be refactored): ```swift func openDefaultApp(subject: String, body: String, recipients: [String]) -> Bool { var components = URLComponents(string: "mailto:\(recipients.joined(separator: ";").addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) ?? "")")! components.queryItems = [ URLQueryItem(name: "subject", value: subject), URLQueryItem(name: "body", value: body), ] if let url = components.url, UIApplication.shared.canOpenURL(url) { UIApplication.shared.open(url) return true } return false } ``` As a result: <img width="300" alt="image" src="https://github.com/organicmaps/organicmaps/assets/79797627/0fe68515-41c0-422d-aac6-0ea2289ab71b">

iphone/Maps/UI/Help/AboutController/AboutController.swift

					
				@ -519,4 +522,3 @@

				// MARK: - UIStackView + AddArrangedSubviewWithSeparator

				private extension UIStackView {

				  func addArrangedSubviewWithSeparator(_ view: UIView) {

kirylkaveryn commented

2024-04-08 08:18:32 +00:00

Maybe dont' remove MWMMailViewController?
Lets make as we discussed earlier in the issue:
if (canOpenDefaultApp) {
open default...
} else MWMMailViewController.canSendMail() {
send with internal mail controller
} else {
show error
}

Maybe dont' remove MWMMailViewController? Lets make as we discussed earlier in the issue: if (canOpenDefaultApp) { open default... } else MWMMailViewController.canSendMail() { send with internal mail controller } else { show error }

v-lozko (Migrated from github.com) reviewed 2024-04-08 14:42:17 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

					
				@ -519,4 +522,3 @@

				// MARK: - UIStackView + AddArrangedSubviewWithSeparator

				private extension UIStackView {

				  func addArrangedSubviewWithSeparator(_ view: UIView) {

v-lozko (Migrated from github.com) commented

2024-04-08 14:42:17 +00:00

So if I'm not mistaken it will never reach the first else and execute the MWMMailViewController.canSendMail() because it will always open with the default email app, even if that is the native app. If it can't open with URL then it shouldn't be able to open with .canSendMail() and would go to the error

kirylkaveryn approved these changes 2024-04-09 17:20:24 +00:00

biodranik (Migrated from github.com) reviewed 2024-04-09 19:08:23 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

					
				@ -484,0 +489,4 @@

				        URLQueryItem(name: "body", value: body.replacingOccurrences(of: "\n", with: "\r\n")),

				      ]

				      if let url = components?.url, UIApplication.shared.canOpenURL(url) {

biodranik (Migrated from github.com) commented

2024-04-09 19:08:23 +00:00

When does it return false?

v-lozko (Migrated from github.com) reviewed 2024-04-18 12:12:01 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

					
				@ -484,0 +489,4 @@

				        URLQueryItem(name: "body", value: body.replacingOccurrences(of: "\n", with: "\r\n")),

				      ]

				      if let url = components?.url, UIApplication.shared.canOpenURL(url) {

v-lozko (Migrated from github.com) commented

2024-04-09 19:14:13 +00:00

when UIApplication.shared.canOpenURL(url) returns false or components?.url doesnt construct properly

v-lozko (Migrated from github.com) commented

2024-04-16 12:17:44 +00:00

@biodranik is there anything else needed for this?

biodranik (Migrated from github.com) approved these changes 2024-04-18 12:29:33 +00:00

biodranik (Migrated from github.com) left a comment

LGTM

iphone/Maps/UI/Help/AboutController/AboutController.swift

biodranik (Migrated from github.com) commented

2024-04-18 12:29:25 +00:00

Did you test the else case here? Does it work?

v-lozko (Migrated from github.com) reviewed 2024-04-19 15:00:09 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

v-lozko (Migrated from github.com) commented

2024-04-19 15:00:09 +00:00

honestly, i'm not sure of a case when it would be triggered. There will always be a default app even if its the native app. This will trigger the openDefaultApp

biodranik (Migrated from github.com) reviewed 2024-04-19 20:48:59 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

biodranik (Migrated from github.com) commented

2024-04-19 20:48:59 +00:00

Then do we need to keep that old code?

v-lozko (Migrated from github.com) reviewed 2024-04-20 11:05:04 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

v-lozko (Migrated from github.com) commented

2024-04-20 11:05:04 +00:00

i had initially removed it but @kirylkaveryn had asked that perhaps i dont. I can remove it, just let me know

kirylkaveryn reviewed 2024-04-20 11:08:48 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

kirylkaveryn commented

2024-04-20 11:08:48 +00:00

@v-lozko let's remove it if you are sure that this code will never be triggered

v-lozko (Migrated from github.com) reviewed 2024-04-20 11:24:09 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

v-lozko (Migrated from github.com) commented

2024-04-20 11:24:09 +00:00

Done, I also removed the delegate alongside it as it'll no longer be needed

biodranik (Migrated from github.com) reviewed 2024-04-20 11:32:00 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! So the only case when user will see the error dialog is when no any mail client is installed, right? Does it work properly on iOS 12 too?

iphone/Maps/UI/Help/AboutController/AboutController.swift

biodranik (Migrated from github.com) commented

2024-04-20 11:31:16 +00:00

    if openDefaultMailApp(subject: subject, body: body, recipients: toRecipients) {

```suggestion if openDefaultMailApp(subject: subject, body: body, recipients: toRecipients) { ```

v-lozko (Migrated from github.com) reviewed 2024-04-20 11:44:29 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

v-lozko (Migrated from github.com) commented

2024-04-20 11:44:29 +00:00

are you updating the name of the function on line 485 as well? I see only 510 being updated, and it would not work if we just change this line.

biodranik (Migrated from github.com) reviewed 2024-04-20 12:12:39 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

biodranik (Migrated from github.com) commented

2024-04-20 12:12:39 +00:00

As a reviewer, I'm suggesting to use a more clear name of the function. It's an author's responsibility to make it work (or disagree with arguments).

v-lozko (Migrated from github.com) reviewed 2024-04-20 12:18:49 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

v-lozko (Migrated from github.com) commented

2024-04-20 12:18:49 +00:00

Sorry, i do agree with the changes. I was thinking to sign and accept the changes from here to simplify. In any case i did update the naming

biodranik (Migrated from github.com) approved these changes 2024-04-24 22:36:51 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! A minor nit )

iphone/Maps/UI/Help/AboutController/AboutController.swift

biodranik (Migrated from github.com) commented

2024-04-24 22:36:36 +00:00

    if openDefaultMailApp(subject: subject, body: body, recipients: toRecipients) {

```suggestion if openDefaultMailApp(subject: subject, body: body, recipients: toRecipients) { ```

v-lozko (Migrated from github.com) reviewed 2024-04-24 22:45:06 +00:00

iphone/Maps/UI/Help/AboutController/AboutController.swift

v-lozko (Migrated from github.com) commented

2024-04-24 22:45:06 +00:00

No problem, signed off and committed.

This repo is archived. You cannot comment on pull requests.

No reviewers

kirylkaveryn

biodranik

No labels

Android Automotive (AAOS)

API

AppGallery

AppStore

Battery and Performance

External Map Datasets

F-Droid

Fonts

Frequently User Reported

No milestone

No project

No assignees

2 participants

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#7862

Rows
Columns