ALL-3936/Package restructure #55
Conversation
…d reduce scope where possible.
|
|
||
| import java.io.IOException; | ||
|
|
||
| final class ExoPlayerErrorFactory { | ||
| public final class ExoPlayerErrorFactory { |
There was a problem hiding this comment.
Is this ok that is available from the client?
There was a problem hiding this comment.
I don't see the problem with this being accessible from the client. They will only get a specific Player.PlayerError based on an Exception. They can't extend it or anything else.
There was a problem hiding this comment.
If this is public a client can do ExoPlayerErrorFactory.errorFor(...). Is this part of the public API?
There was a problem hiding this comment.
We opted to rename as Mapper and move to the package where it is used 6b400cf
| @@ -12,7 +12,7 @@ public TextCues(List<Cue> cues) { | |||
| this.cues = cues; | |||
| } | |||
|
|
|||
| List<Cue> cues() { | |||
| public List<Cue> cues() { | |||
There was a problem hiding this comment.
This class needs to be public facing?
There was a problem hiding this comment.
It wouldn't need to be if we collapsed some more packages but all of our models now belong in the model package meaning they need to be public. Personally, I would find it strange if we didn't expose our NoPlayer models.
There was a problem hiding this comment.
If the client needs to have those models then ok, but if it doesn't then these models become a contract with the clients
There was a problem hiding this comment.
TextCues is part of the NoPlayer contract as it's used in PlayerView (although in it's current form it's leaking exoplayer but this is fixed in #56)
Mecharyry
changed the base branch from
ALL-3936/ensure_target_api
to
exo-player-2
juankysoriano
requested review from
juankysoriano
and removed request for
joetimmins
| @@ -391,7 +391,7 @@ public void whenGettingAudioTracks_thenDelegatesToTrackSelector() { | |||
| @Mock | |||
| DrmSessionCreator drmSessionCreator; | |||
| @Mock | |||
| ExoPlayerDrmSessionEventListener drmSessionEventListener; | |||
| DefaultDrmSessionManager.EventListener drmSessionEventListener; | |||
There was a problem hiding this comment.
I'm not tooo fussed, it's a test 🍷 unless you really want it? 🌵
There was a problem hiding this comment.
I guess you feel super good after doing this!!!! all those reduced scopes!!!
Leaving to @joetimmins
| import java.io.IOException; | ||
| import java.nio.charset.Charset; | ||
|
|
||
| class ProvisionExecutorImpl implements ProvisionExecutor { |
There was a problem hiding this comment.
Can we call this something that doesn't end in impl? HttpPostingProvisionExecutor or something maybe?
Problem
Lots and lots of public classes that could be package if they were in the right place.
Solution
Move classes to logically grouped areas and make them package scope.
Test(s) added
No, just updating packages.
Screenshots
No UI changes.
Paired with
@ouchadam