This repository has been archived by the owner on May 10, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 440
Fix #3673 - Implements CarPlay + Refactor Playlist #3887
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Brandon-T
force-pushed
the
feature/Playlist-Carplay
branch
2 times, most recently
from
July 9, 2021 15:29
1392f5a
to
4f35dd8
Compare
Brandon-T
force-pushed
the
feature/Playlist-Carplay
branch
from
July 15, 2021 17:40
90723c2
to
995ac57
Compare
iccub
suggested changes
Aug 2, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left few nitpick comments, will run this branch later
Besides that I think there are 2 areas worth looking into:
- See if we can remove tight coupling with BVC/App Delegate. In the future we will move to using SceneDelegates, which means we should think what code will be shared and what will be different for each opened window/scene. You can see if we can utilize delegates or passing bvc in constructor if we ever need access to it
- I had a feeling that some of code between regular player and car play player can be shared. You can see if it makes sense in any place. Of course it may not make sense to share code even if it looks similar. I'm not pro DRY in every case, just something to think about.
Client/Frontend/Browser/Playlist/Controller/PlaylistCarplayController.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Browser/Playlist/Controller/PlaylistCarplayController.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Browser/Playlist/Controller/PlaylistCarplayController.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Browser/Playlist/Controller/PlaylistCarplayController.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Browser/Playlist/Controller/PlaylistCarplayController.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Browser/Playlist/Utilities/PlaylistMediaStreamer.swift
Outdated
Show resolved
Hide resolved
Client/Frontend/Browser/Playlist/Utilities/PlaylistStatusObserver.swift
Outdated
Show resolved
Hide resolved
iccub
reviewed
Aug 6, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feedback review looks good to me, I will run this branch and leave functional review next week
Brandon-T
force-pushed
the
feature/Playlist-Carplay
branch
2 times, most recently
from
August 13, 2021 15:34
76d8ae5
to
6a69081
Compare
FYI after we merge this we must update provisioning profiles to include the CarPlay entitlement otherwise the app will fail to sign in Release builds. Apple has made us entitlements that include both Default Web Browser and CarPlay. |
Brandon-T
force-pushed
the
feature/Playlist-Carplay
branch
4 times, most recently
from
August 25, 2021 13:44
55341ae
to
d44c06f
Compare
Brandon-T
force-pushed
the
feature/Playlist-Carplay
branch
from
August 26, 2021 14:47
d44c06f
to
6e9a23d
Compare
iccub
approved these changes
Aug 27, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of Changes
This pull request fixes #3673
Submitter Checklist:
NSLocalizableString()
Test Plan:
Screenshots:
Reviewer Checklist:
QA/(Yes|No)
release-notes/(include|exclude)
bug
/enhancement