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

Adopt iOS 17 @Observable? #424

Closed
2 tasks done
defagos opened this issue Jun 20, 2023 · 6 comments
Closed
2 tasks done

Adopt iOS 17 @Observable? #424

defagos opened this issue Jun 20, 2023 · 6 comments
Labels
question Further information is requested stale This was not recently worked on

Comments

@defagos
Copy link
Member

defagos commented Jun 20, 2023

Detailed question

Apple introduced @Observable at WWDC 2023. Should we adopt it for Pillarbox player?

Can your answer be found elsewhere?

  • I have searched existing issues and found no similar question.

  • I have browsed the available documentation and found no answer to my question.

@defagos defagos added question Further information is requested triage Requires triage first and removed triage Requires triage first labels Jun 20, 2023
@defagos
Copy link
Member Author

defagos commented Jun 20, 2023

It is probably a bad idea to adopt @Observable for Player, as a major difference between @Observable and @ObservableObject is that the latter is inherently reactive, which is exactly what is needed for Pillarbox player.

We could probably use @Observable for non-necessarily reactive components like ProgressTracker or VisibilityTracker, though, but only when Pillarbox officially supports iOS 17+.

@defagos
Copy link
Member Author

defagos commented Aug 19, 2023

Pitch discussions are extremely interesting.

I remain convinced that we should keep ObservableObject which best suits our needs.

@defagos
Copy link
Member Author

defagos commented Oct 3, 2023

Once we have finished #559 we might be able to investigate @Observable support again. We will likely reach a state where no state is publicly published, with a publisher delivering updates instead. We should:

  1. Hack @Observable support. According to @walid properties deeply nested in player properties can be automatically observed, leading to partial body updates. With hacked support we should be able to verify the UI behavior and the number of refreshes obtained pretty easily.
  2. If the approach is successful we should transform our current Player into an implementation class _Player for iOS 16 ObservableObject support implemented by another Player class, simply forwarding methods to _Player. On iOS 17 this class would be marked as @Observable, probably with the help of the preprocessor. Two naive approaches:
    • Use the preprocesor to strip @Observable and associated attributes from Player on iOS 16, and to rename Player as _Player. Provide the separate ObservablePlayer implementation on iOS 16, conforming to ObservableObject and forwarding calls to the _Player implementation.
    • On iOS 17 use Player as is. We can still provide both Player and ObservablePlayer for a while on iOS 17 so that applications can use one of them and migrate when they want to the new observation approach, mostly likely when their deployment target is iOS 17+.

To provide this API officially, though, we should likely wait until leaks in modals have been fixed.

Copy link

github-actions bot commented Jan 2, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 14 days or if the stale label is not removed.

@github-actions github-actions bot added the stale This was not recently worked on label Jan 2, 2024
Copy link

This issue has been automatically closed because it has not had recent activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 16, 2024
@defagos
Copy link
Member Author

defagos commented Jul 6, 2024

Adopting @Observable might be the best approach to move to Swift 6, see #908. Since iOS 18 is around the corner I would suggest we adopt @Observable if this makes sense and drop iOS 16 compatibility, according to our support policy.

Work likely required:

  • Make Player observable. Possibly identical for other observable classes like ProgressTracker, for example.
  • Keep publishers and reactive nature intact.
  • Wire publishers to properties with weakAssign(to:on:) instead of assign(to:).
  • Think if we should have a strong guarantee that all events are delivered on the same queue (serial queue managed by the player, not necessarily main).
  • Check if property slicing is still required. Since observation follows changing properties it might not be necessary anymore to isolate buffering state changes, for example, to avoid unnecessary refreshes. The onReceive modifier we have added might therefore be droppable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested stale This was not recently worked on
Projects
None yet
Development

No branches or pull requests

1 participant