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

Applying MVVM and Moya on some screens, Changed filter structure #20

Merged
merged 15 commits into from
May 29, 2021

Conversation

PangMo5
Copy link
Member

@PangMo5 PangMo5 commented May 27, 2021

Changed

  • Add JellyfinAPI
  • Add Filter struct, API related enums
  • Add LibraryListViewModel, LibraryViewModel, LibrarySearchViewModel
  • Change a specific toolbar to a navigation bar item.
  • Separate LibraryView and LibraryListView.
  • Change to present SettingsView as sheet

Known issues

- UI cannot be updated after loading when entering LibraryView in MovieItemView and EpisodeItemView.
(Since it works normally when entering from another screen, there seems to be a problem in the life cycle of the MovieItemView and EpisodeItemView.)
-> Life cycle issues of SwiftUI(NavigationLink) solved as Lazy

And it seems that there are a lot of @State, @EnvironmentObject, and ObservableObject that are not currently used in a lot of code. It would be nice to increase the performance by refactoring the related code.

I'm sorry that the commits weren't cleaned up as I modified a lot of code at once.

@acvigue acvigue self-requested a review May 27, 2021 14:25
@acvigue
Copy link
Member

acvigue commented May 27, 2021

All library requests return 401

@PangMo5
Copy link
Member Author

PangMo5 commented May 27, 2021

All library requests return 401

It is normal on the simulator, but it seems to return 401 on the actual device.
Could you please give me any Jellyfin API documentation?

@acvigue
Copy link
Member

acvigue commented May 27, 2021

Try removing the backslashes from the authheader

@PangMo5
Copy link
Member Author

PangMo5 commented May 27, 2021

Try removing the backslashes from the authheader

I checked the authHeader, but there was no backslash.

MediaBrowser Client="SwiftFin", Device="PangMo5’s iPad", DeviceId="ED8E8ABB-B796-41BC-AFB7-1A2895C0370A", Version="1.0.0", Token="xxxxxxxxxxxxxxxxxxxxxxxx"

The curious thing is that it works fine in the simulator.

@acvigue
Copy link
Member

acvigue commented May 27, 2021 via email

@PangMo5
Copy link
Member Author

PangMo5 commented May 27, 2021

I found the cause.
There seems to be a conflict with the name of the actual device.
If you remove the Device from the authHeader, it works normally.

@acvigue
Copy link
Member

acvigue commented May 27, 2021

Try escaping the Device name - I'd rather keep it in as it allows users to see the device name in the web dashboard

@PangMo5
Copy link
Member Author

PangMo5 commented May 27, 2021

Try escaping the Device name - I'd rather keep it in as it allows users to see the device name in the web dashboard

Yes it is important.
Let me find out if there is another way.

@PangMo5
Copy link
Member Author

PangMo5 commented May 27, 2021

@acvigue
Copy link
Member

acvigue commented May 27, 2021

Please resolve the merge conflict in project.pbxproj and rebase :)

@PangMo5
Copy link
Member Author

PangMo5 commented May 27, 2021

Please resolve the merge conflict in project.pbxproj and rebase :)

Resolved conflic. Please merge it.

@acvigue acvigue added the enhancement New feature or request label May 27, 2021
@PangMo5
Copy link
Member Author

PangMo5 commented May 28, 2021

Please don't merge this yet.
Let me solve the Know issues.

@acvigue
Copy link
Member

acvigue commented May 28, 2021

Okay, take your time :)

@PangMo5
Copy link
Member Author

PangMo5 commented May 28, 2021

@acvigue fixed all issues, Please merge it :D

@acvigue
Copy link
Member

acvigue commented May 28, 2021

Hey! Thanks. I'll look at this when I get off work tmrw.

@acvigue
Copy link
Member

acvigue commented May 28, 2021

what exactly is LazyView.swift?

@PangMo5
Copy link
Member Author

PangMo5 commented May 28, 2021

what exactly is LazyView.swift?

Like other Views in SwiftUI, NavigationLink is not lazy.
Therefore, Life cycle problems can occur and LazyView helps this.

@acvigue
Copy link
Member

acvigue commented May 28, 2021

i see - what problems occur without lazyview. also if the extension is only used in one file, it shouldnt be abstracted away for the sole purpose of abstraction IMO

@PangMo5
Copy link
Member Author

PangMo5 commented May 28, 2021

i see - what problems occur without lazyview. also if the extension is only used in one file, it shouldnt be abstracted away for the sole purpose of abstraction IMO

It doesn't matter if you don't abstract it in a personal project.
But I think it's different in massive projects.
Because extensions of the same function can continue to occur.
I think this method can be used many times later.

@acvigue
Copy link
Member

acvigue commented May 28, 2021

lgtm!
only comment -buttons are way too close together on library view

@acvigue
Copy link
Member

acvigue commented May 29, 2021

will merge once buttons are respaced :)

@PangMo5
Copy link
Member Author

PangMo5 commented May 29, 2021

will merge once buttons are respaced :)

What does the spacing of the buttons mean? Are you talking about the spacing of the navigation buttons?

GlobalData conform Equatable
Change the conditions for some screens onAppear
# Conflicts:
#	JellyfinPlayer/EpisodeItemView.swift
#	JellyfinPlayer/LibraryView.swift
#	JellyfinPlayer/MovieItemView.swift
#	JellyfinPlayer/SeasonItemView.swift
@PangMo5
Copy link
Member Author

PangMo5 commented May 29, 2021

Conflict occurred in LibraryView during Merge.
It was too big conflic, so I chose all of them as my code.
Please check if there is a missing code.

@acvigue
Copy link
Member

acvigue commented May 29, 2021

lgtm

@acvigue acvigue closed this May 29, 2021
@acvigue acvigue reopened this May 29, 2021
@acvigue acvigue merged commit db44b48 into jellyfin:main May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants