search

jellyfin / Swiftfin

Native Jellyfin Client for iOS and tvOS
Mozilla Public License 2.0
2.55k stars 278 forks source link

Performance improvements and structural recommendations. #28

Closed PangMo5 closed 3 years ago

PangMo5 commented 3 years ago

Currently using meaningless ObservableObject and Published in DetailItem, etc.. model

Overall, after creating a ViewModel that conform ObservableObject, it would be better to go in the direction of creating the data to be displayed in the view using the Published property wrapper. And the current method can introduce unintended side effects. Please refer to LibraryViewModel, etc.

Use Struct or Enum instead of Classes for model objects.

For the model object, it is more appropriate to use the value type Struct rather than the reference type Class.

And when using Published of ObservableObject, when using a reference type model, objectWillChange is called only when the object itself is changed. On the other hand, when using a value type model, objectWillChange is called even if only the internal property value is changed. https://developer.apple.com/documentation/combine/observableobject

It would be nice to fit Swift's default conventions.

https://swift.org/documentation/api-design-guidelines/

The convention when communicating with the API is to utilize KeyDecodingStrategy. https://developer.apple.com/documentation/foundation/jsondecoder/keydecodingstrategy

In addition, I will promote the introduction of SwiftFormat. https://github.com/nicklockwood/SwiftFormat

I think it's better not to use EnvironmentObject for business logic like GlobalData.

EnvironmentObject passed after init in View, it is difficult to pass to ViewModel. It is also difficult to inject when EnvironmentObject is updated. I think seems better to just use Singleton.

acvigue commented 3 years ago

we should migrate to the generated openapi client (which uses Alamofire) : https://github.com/jellyfin/jellyfin-sdk-swift

PangMo5 commented 3 years ago

we should migrate to the generated openapi client (which uses Alamofire) : https://github.com/jellyfin/jellyfin-sdk-swift

Oh very good news. May I design the base?

acvigue commented 3 years ago

Sure! I just generated it and haven't really played around with it yet, but it should have auto generated some docs

acvigue commented 3 years ago

Don't waste your time with the generated client - it doesn't work. There's a couple bugs in how the generator works.

acvigue commented 3 years ago

I've been trying to figure it out all day

PangMo5 commented 3 years ago

Okay. Let me know when the work is done.

acvigue commented 3 years ago

it will be a long while

Sent from my iPhone

On Jun 3, 2021, at 12:08 AM, Kwangmin Bae @.***> wrote:

 Okay. Let me know when the work is done.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

PangMo5 commented 3 years ago

Oh i see.. I also tried to create it using https://github.com/openapitools/openapi-generator or https://github.com/yonaskolb/SwagGen, but the API that builds normally is not created.

If it doesn't work, it seems that we have to solve the build error every time we update the version after creating it with https://github.com/openapitools/openapi-generator.

acvigue commented 3 years ago

It’s an issue with both the actual OAS spec and the generator. Problem is - we need both stable and nightly clients built daily and put in the sdk repo (workflow bot) so we couldn’t manually edit them

Sent from my iPhone

On Jun 3, 2021, at 1:24 AM, Kwangmin Bae @.***> wrote:

 Oh i see.. I also tried to create it using https://github.com/openapitools/openapi-generator or https://github.com/yonaskolb/SwagGen, but the API that builds normally is not created.

If it doesn't work, it seems that we have to solve the build error every time we update the version after creating it with https://github.com/openapitools/openapi-generator.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

acvigue commented 3 years ago

SwagGen causes a seg fault OpenAPI generator works but the build fails with “error 4”

Sent from my iPhone

On Jun 3, 2021, at 1:24 AM, Kwangmin Bae @.***> wrote:

 Oh i see.. I also tried to create it using https://github.com/openapitools/openapi-generator or https://github.com/yonaskolb/SwagGen, but the API that builds normally is not created.

If it doesn't work, it seems that we have to solve the build error every time we update the version after creating it with https://github.com/openapitools/openapi-generator.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

PangMo5 commented 3 years ago

https://github.com/Azure/autorest.swift Can you try with this?

acvigue commented 3 years ago

https://github.com/Azure/autorest.swift Can you try with this?

Doesn't work either. Tried with stable and unstable

PangMo5 commented 3 years ago

https://github.com/swagger-api/swagger-codegen How about this tool?

PangMo5 commented 3 years ago

https://github.com/swagger-api/swagger-codegen How about this tool?

I tried this Some property types are being created as Any. It was the same even if I put the type-mappings parameter.

swagger-codegen generate -i https://api.jellyfin.org/openapi/jellyfin-openapi-stable.json -l swift5 --type-mappings object=JSONValue

PangMo5 commented 3 years ago

updated bump