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

Custom URL Scheme Implementation #657

Merged

Conversation

hassan31
Copy link
Contributor

@hassan31 hassan31 commented Sep 6, 2024

Implemented custom url scheme to support launching the Quran Application from outside the application
For example, enter this in browser quran-ios:// and it will redirect you to the Quran Application

Example Video

RPReplay_Final1725599281.MP4

Copy link

@iman2153 iman2153 left a comment

Choose a reason for hiding this comment

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

lgtm

@hassan31
Copy link
Contributor Author

hassan31 commented Sep 7, 2024

@mohamede1945 could you please review this pr and approve so that it can be merged. Thanks

@mohamede1945
Copy link
Collaborator

Salam alikom Muhammad, thank you for the contribution. But the code is empty. There's nothing implemented to parse the query parameters and navigate to a specific Ayah as mentioned in #348.

@hassan31
Copy link
Contributor Author

hassan31 commented Sep 7, 2024

@mohamede1945 Wa Alaikumuslaam, Yes, this is the partial implementation, in which I have launched the application from outside link. In followup pr I will write the code to parse. So if you could approve and merge that would be really great.

Copy link
Collaborator

@mohamede1945 mohamede1945 left a comment

Choose a reason for hiding this comment

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

Sounds good. I've added a couple of comments, please take a look. Also, please make sure to run make format-autocorrect to fix formatting issues.

Thank you!


func application(_ app: UIApplication, open url: URL, options: [UIApplication.OpenURLOptionsKey: Any] = [:]) -> Bool {
// Check if the URL scheme matches the custom scheme
if url.scheme == "quran-ios" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's handle "quran" and "quran-ios" schemes please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@@ -49,6 +49,27 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
let downloadManager = container.downloadManager
downloadManager.setBackgroundSessionCompletion(completionHandler)
}

func application(_ app: UIApplication, open url: URL, options: [UIApplication.OpenURLOptionsKey: Any] = [:]) -> Bool {
// Check if the URL scheme matches the custom scheme
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this redundant comment. The code is clear. Same with the other inline comments below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

// Check if the URL scheme matches the custom scheme
if url.scheme == "quran-ios" {
// Parse the URL to determine the desired action
let path = url.host ?? ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

host is not the path. We should look at the url.path rather than the host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

return false
}

private func handleCustomURL(path: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

handle seems very generic. I would rename it to navigateTo(path: String). Also let's make this method return false if it can't parse and navigate to the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

<string>com.quran.QuranEngineApp</string>
<key>CFBundleURLSchemes</key>
<array>
<string>quran-ios</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add an entry for quran scheme please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed

@mohamede1945
Copy link
Collaborator

Oh actually, please move the validation, parsing and navigation code to the QuranEngine instead of the example app. The example app would simply just implement the delegate method but then calls the QuranEngine to do the actual logic, this way the code can be shared among different apps.

@hassan31
Copy link
Contributor Author

hassan31 commented Sep 8, 2024

Oh actually, please move the validation, parsing and navigation code to the QuranEngine instead of the example app. The example app would simply just implement the delegate method but then calls the QuranEngine to do the actual logic, this way the code can be shared among different apps.

AOA @mohamede1945 I have addressed your comments, kindly review again. Also regarding move the validation, parsing and navigation code to QuranEngine, i can see the same code there, but in pr it reflects in Example/QuranEngine app. May be if you guide how to move then I would do. Please check below screenshot.

Screenshot 2024-09-08 at 12 16 07 AM

@mohamede1945
Copy link
Collaborator

You see here how the code in the example project delegates the business logic to the DownloadManager and ReadingResourcesService. These live in the QuranEngine library not in the Example project. The example project just delegates the call to these services.

In url scheme navigation logic, I think it makes sense to live in the AppStructureFeature.

@hassan31
Copy link
Contributor Author

hassan31 commented Sep 8, 2024

You see here how the code in the example project delegates the business logic to the DownloadManager and ReadingResourcesService. These live in the QuranEngine library not in the Example project. The example project just delegates the call to these services.

In url scheme navigation logic, I think it makes sense to live in the AppStructureFeature.

@mohamede1945 can we connect through some communication tools, like slack or google meet, I wanted to learn more about the structure?

@hassan31
Copy link
Contributor Author

hassan31 commented Sep 9, 2024

@mohamede1945 are you still available to review the latest changes?

@mohamede1945
Copy link
Collaborator

Jazak Allah khyrn for the changes. I'm going to merge it after CI finishes. You can reach out to me on discord with the same username used here in github.

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 39.69%. Comparing base (d9fc366) to head (5223a5b).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...res/AppStructureFeature/Launch/LaunchBuilder.swift 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #657      +/-   ##
==========================================
- Coverage   40.92%   39.69%   -1.24%     
==========================================
  Files         525      526       +1     
  Lines       20880    19453    -1427     
==========================================
- Hits         8546     7721     -825     
+ Misses      12334    11732     -602     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hassan31
Copy link
Contributor Author

hassan31 commented Sep 10, 2024

Jazak Allah khyrn for the changes. I'm going to merge it after CI finishes. You can reach out to me on discord with the same username used here in github.

Merge is blocked, because of some checks are not successful. I also installed the swiftlint, but looks good on my side. May be you can help to find what is wrong with which format.

Also, I sent you request on discord. Please accept. Jazakallah Khair.

@mohamede1945 mohamede1945 merged commit 7bd3f18 into quran:main Sep 11, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants