Apply Coordinator Pattern and Add Deep-Links #155

PangMo5 · 2021-08-21T11:02:57Z

With the Coordinator pattern,

Helps to single-responsibility principle (SRP).
It collects scattered navigation processing codes into one place.
Makes the View lighter.
etc...

Coordinators

Deep-Links

jellyfin://Users/{UserID}/Items/{ItemID}

Feel free to leave any advice on the current structure!

Add ConnectToServerCoodinator Add HomeCoordinator Add LibraryListCoordinator Add MainCoordinator Add MainTabCoordinator

LePips · 2021-08-21T16:36:58Z

This work looks great! Are you referencing any articles or tutorials for this?

We need to make some contribution and developer docs for this project so that us and other contributors know what architectures are being used and what we have discussed and decided upon. We don't want someone in the future re-writing all of this on their own.

PangMo5 · 2021-08-21T20:12:33Z

This work looks great! Are you referencing any articles or tutorials for this?

I'm currently using https://github.com/rundfunk47/stinsen and referencing on an example and docs of that project.

I think if you look at Swiftfin's code after the work is complete, you can easily understand the action and structure of each coordinator.

acvigue · 2021-08-23T01:19:43Z

looks great!

LePips · 2021-08-24T02:51:16Z

@PangMo5 dont mean to bug, have you done any more work on this? I want to do view model refactoring after this. If your branch is up to date, I can help work on this if you would like. If not, that's okay

PangMo5 · 2021-08-24T03:11:11Z

@PangMo5 dont mean to bug, have you done any more work on this? I want to do view model refactoring after this. If your branch is up to date, I can help work on this if you would like. If not, that's okay

First, the branch is up to date.

But I'll proceed for now. It won't take too long.
I'll mention it if I need help. Thanks.

Add LibraryCoordinator Add SearchCoordinator Add SettingsCoordinator Update HomeCoordinator Update LibraryListCoordinator

rename router

# Conflicts: # JellyfinPlayer/SettingsView.swift # JellyfinPlayer/VideoPlayer.swift

PangMo5 · 2021-08-25T11:21:58Z

The current implementation is complete, but the Coordinator has many duplicate codes and DeepLink does not work as expected.

I created related question in Stinsen project.

PangMo5 · 2021-08-25T13:08:17Z

For now, I'm changed a PR state to get advice.

LePips · 2021-08-25T16:13:22Z

I see the discussion going on in your issue and that he needs to do some work with a coordinator in a navigation stack.

If this satisfies the needs for deep linking, I'm good with and support this work. I'm okay with the duplicate code here since it's due to a protocol.

Shared/Singleton/AppURLHandler.swift

Shared/ViewModels/ConnectToServerViewModel.swift

JellyfinPlayer.xcodeproj/project.pbxproj

JellyfinPlayer/SettingsView.swift

PangMo5 · 2021-08-25T17:31:08Z

If this satisfies the needs for deep linking, I'm good with and support this work. I'm okay with the duplicate code here since it's due to a protocol.

I need to think more about the deep link structure :(
I'd appreciate your help!

PangMo5 · 2021-09-21T20:33:00Z

I don't see widgets in my simulator, maybe it's a simulator's problem.
If you can, please check whether the deep-link is working properly in widgets.

LePips · 2021-09-21T20:42:18Z

JellyfinPlayer/Singleton/AppURLHandler.swift

+           let userID = url.pathComponents[safe: 1],
+           let itemID = url.pathComponents[safe: 3]
+        {
+            // It would be nice if the ItemViewModel could be initialized to id later.


I will keep this in mind when I refactor how we handle media.

LePips · 2021-09-21T20:45:18Z

I don't see widgets in my simulator, maybe it's a simulator's problem.
If you can, please check whether the deep-link is working properly in widgets.

You have to search for the app and Jellyfin will be a result. Just a bug of the simulator.

PangMo5 · 2021-09-21T20:49:40Z

You have to search for the app and Jellyfin will be a result. Just a bug of the simulator.

I can't find it :(

LePips · 2021-09-21T20:50:50Z

I can't find it :(

That's strange, try the iOS 15 simulator. The search is working for me there

PangMo5 · 2021-09-21T20:58:43Z

That's strange, try the iOS 15 simulator. The search is working for me there

It's the same in 15 🤔
If you see the widgets, please test it :p

LePips · 2021-09-21T21:01:03Z

Sorry, I'm going to have to take back my approval. The app is crashing upon signing out. This is because of a forced bang on our SessionManager object (which we shouldn't necessarily use and that I am refactoring).

~~However, the app is trying to build SettingsView again after signing out, which is where the crash is occurring.~~

LePips · 2021-09-21T21:06:51Z

Ignore last comment. In the meantime, do a:

                    if let user = SessionManager.current.user {
                        Text(user.username ?? "")
                            .foregroundColor(.jellyfinPurple)
                    }

in SettingsView, starting on line 34.

As I have said, I'll fix all Session/server/user stuff.

PangMo5 · 2021-09-21T21:08:26Z

Interestingly, it seems to be a problem in only iOS 15. lol
Anyway I'll fix it.

LePips · 2021-09-21T21:08:52Z

Interestingly, it seems to be a problem in only iOS 15. lol
Anyway I'll fix it.

Oh, that is interesting. Sounds good to me

LePips · 2021-09-21T21:35:24Z

Widgets work great on their own and are bugged out a little bit because of, guess what, the way we handle sessions/server/user. This doesn't stop the work here from being merged.

# Conflicts: # JellyfinPlayer.xcodeproj/project.pbxproj # JellyfinPlayer.xcworkspace/xcshareddata/swiftpm/Package.resolved

fix build error

sonarcloud · 2021-09-23T09:09:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
41 Code Smells

No Coverage information
1.2% Duplication

LePips · 2021-09-30T01:59:02Z

@acvigue Could this either be merged or I be given permission to merge? I think having this work in so that jhays can implement it in tvOS would be great.

anthonylavado

Approving based on comment from @LePips.

Add Stinsen

3a090aa

Add ConnectToServerCoodinator Add HomeCoordinator Add LibraryListCoordinator Add MainCoordinator Add MainTabCoordinator

PangMo5 added the enhancement New feature or request label Aug 21, 2021

PangMo5 self-assigned this Aug 21, 2021

PangMo5 linked an issue Aug 21, 2021 that may be closed by this pull request

View route improvement and deep-linking support #125

Closed

PangMo5 changed the title ~~Apply Coordinator Patterns and Add Deep-Links~~ Apply Coordinator Pattern and Add Deep-Links Aug 21, 2021

Merge branch 'main' into PangMo5/coordinator-and-deep-link

6c5867c

Merge branch 'main' into PangMo5/coordinator-and-deep-link

4d6ca79

PangMo5 added 3 commits August 25, 2021 14:26

Add FilterCoordinator

0640e70

Add LibraryCoordinator Add SearchCoordinator Add SettingsCoordinator Update HomeCoordinator Update LibraryListCoordinator

Add ItemCoordinator

74a9302

Add VideoPlayerCoordinator

2aab9df

rename router

PangMo5 linked an issue Aug 25, 2021 that may be closed by this pull request

Next Up Widget #151

Closed

PangMo5 added 5 commits August 25, 2021 17:48

Merge branch 'main' into PangMo5/coordinator-and-deep-link

3ad789f

# Conflicts: # JellyfinPlayer/SettingsView.swift # JellyfinPlayer/VideoPlayer.swift

Respond to main branch

a16d70a

fix tvOS build error

252c7a6

fix build error

b544bd6

Temporarily process deep link from HomeView

5d96de3

PangMo5 marked this pull request as ready for review August 25, 2021 11:22

PangMo5 requested a review from acvigue as a code owner August 25, 2021 11:22

LePips requested changes Aug 25, 2021

View reviewed changes

fix suggested changes

3235f80

PangMo5 marked this pull request as draft August 25, 2021 18:08

PangMo5 marked this pull request as ready for review September 21, 2021 20:28

Merge branch 'main' into PangMo5/coordinator-and-deep-link

c88fabd

remove DeepLinkError

8175395

PangMo5 requested a review from LePips September 21, 2021 20:36

LePips previously approved these changes Sep 21, 2021

View reviewed changes

fix crash when sign out

cd7a58b

PangMo5 dismissed LePips’s stale review via cd7a58b September 21, 2021 21:09

LePips previously approved these changes Sep 21, 2021

View reviewed changes

PangMo5 added 2 commits September 23, 2021 15:58

Merge branch 'main' into PangMo5/coordinator-and-deep-link

1b7e801

# Conflicts: # JellyfinPlayer.xcodeproj/project.pbxproj # JellyfinPlayer.xcworkspace/xcshareddata/swiftpm/Package.resolved

fix tvOS build error

fad8f14

PangMo5 dismissed LePips’s stale review via fad8f14 September 23, 2021 07:35

PangMo5 added 2 commits September 23, 2021 18:00

modify LatestMediaViewModel.requestLatestMedia params

a46ed45

fix build error

fix LibraryFilterView's layout

34dbc2a

LePips self-requested a review September 23, 2021 21:11

LePips approved these changes Sep 23, 2021

View reviewed changes

anthonylavado approved these changes Oct 2, 2021

View reviewed changes

anthonylavado merged commit 084f96b into main Oct 2, 2021

PangMo5 deleted the PangMo5/coordinator-and-deep-link branch October 2, 2021 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply Coordinator Pattern and Add Deep-Links #155

Apply Coordinator Pattern and Add Deep-Links #155

PangMo5 commented Aug 21, 2021 •

edited

LePips commented Aug 21, 2021

PangMo5 commented Aug 21, 2021 •

edited

acvigue commented Aug 23, 2021

LePips commented Aug 24, 2021

PangMo5 commented Aug 24, 2021 •

edited

PangMo5 commented Aug 25, 2021 •

edited

PangMo5 commented Aug 25, 2021 •

edited

LePips commented Aug 25, 2021

PangMo5 commented Aug 25, 2021 •

edited

PangMo5 commented Sep 21, 2021 •

edited

LePips Sep 21, 2021

LePips commented Sep 21, 2021

PangMo5 commented Sep 21, 2021

LePips commented Sep 21, 2021

PangMo5 commented Sep 21, 2021

LePips commented Sep 21, 2021 •

edited

LePips commented Sep 21, 2021

PangMo5 commented Sep 21, 2021

LePips commented Sep 21, 2021

LePips commented Sep 21, 2021

sonarcloud bot commented Sep 23, 2021

LePips commented Sep 30, 2021

anthonylavado left a comment

Apply Coordinator Pattern and Add Deep-Links #155

Apply Coordinator Pattern and Add Deep-Links #155

Conversation

PangMo5 commented Aug 21, 2021 • edited

Coordinators

Deep-Links

LePips commented Aug 21, 2021

PangMo5 commented Aug 21, 2021 • edited

acvigue commented Aug 23, 2021

LePips commented Aug 24, 2021

PangMo5 commented Aug 24, 2021 • edited

PangMo5 commented Aug 25, 2021 • edited

PangMo5 commented Aug 25, 2021 • edited

LePips commented Aug 25, 2021

PangMo5 commented Aug 25, 2021 • edited

PangMo5 commented Sep 21, 2021 • edited

LePips Sep 21, 2021

Choose a reason for hiding this comment

LePips commented Sep 21, 2021

PangMo5 commented Sep 21, 2021

LePips commented Sep 21, 2021

PangMo5 commented Sep 21, 2021

LePips commented Sep 21, 2021 • edited

LePips commented Sep 21, 2021

PangMo5 commented Sep 21, 2021

LePips commented Sep 21, 2021

LePips commented Sep 21, 2021

sonarcloud bot commented Sep 23, 2021

LePips commented Sep 30, 2021

anthonylavado left a comment

Choose a reason for hiding this comment

PangMo5 commented Aug 21, 2021 •

edited

PangMo5 commented Aug 21, 2021 •

edited

PangMo5 commented Aug 24, 2021 •

edited

PangMo5 commented Aug 25, 2021 •

edited

PangMo5 commented Aug 25, 2021 •

edited

PangMo5 commented Aug 25, 2021 •

edited

PangMo5 commented Sep 21, 2021 •

edited

LePips commented Sep 21, 2021 •

edited