ALL-3936/Internal package #59

Mecharyry · 2017-06-28T14:27:24Z

Problem

As per the discussion in #58 we want to expose an internal package for classes that we are forced to make public but should not be used by clients and therefore do not require JavaDocs.

Solution

Create an internal package and move internally public classes there!

Replace the getters for ErrorListeners etc in the ListenersHolder so that they use the interface rather than the implementation. Allows us to make those implementations package protected so that the only way to add and remove is through the ListenersHolder.

Remove generics on the Heartbeat.Callbacks we always need a Player anyway.

Add the Heartbeat.Callback to the Player because that's where all the other Callbacks and Listeners are always declared here.

Test(s) added

No, just updating the broken packages.

Screenshots

No UI changes.

Paired with

@ouchadam

juankysoriano · 2017-06-28T14:34:47Z

core/src/main/java/com/novoda/noplayer/Player.java


 import java.util.List;
 import java.util.Map;

 public interface Player extends PlayerState {

+
+


juankysoriano

More red than green, and I have seen a few classes with reduced scope. LGTM

…r interface.

Mecharyry · 2017-06-28T14:55:12Z

core/src/main/java/com/novoda/noplayer/Listeners.java

@@ -30,9 +30,9 @@

    void removeBitrateChangedListener(Player.BitrateChangedListener bitrateChangedListener);

-    void addHeartbeatCallback(Heart.Heartbeat.Callback<Player> callback);
+    void addHeartbeatCallback(Player.HeartbeatCallback heartbeatCallback);


Heart.Heartbeat.Callback has been moved to the Player to match the other listeners that the Player supports.

Mecharyry · 2017-06-28T14:55:52Z

core/src/main/java/com/novoda/noplayer/NoPlayerCreator.java

+                    DrmSessionCreator drmSessionCreator = drmSessionCreatorFactory.createFor(drmType, drmHandler);
+                    return noPlayerExoPlayerCreator.createExoPlayer(context, drmSessionCreator, downgradeSecureDecoder);
+                } catch (DrmSessionCreatorException exception) {
+                    throw new UnableToCreatePlayerException(exception);


Mapping an internal exception to a public facing exception.

Mecharyry · 2017-06-28T14:56:36Z

core/src/main/java/com/novoda/noplayer/Player.java

@@ -225,4 +224,9 @@ public void onLoadTimeout() {

        void onLoadTimeout();
    }
+
+    interface HeartbeatCallback {


Moved from Heart.Heartbeat.Callback to match the other callbacks on the Player.

Mecharyry · 2017-06-28T14:57:24Z

core/src/main/java/com/novoda/noplayer/drm/StreamingModularDrm.java

@@ -3,4 +3,23 @@
 public interface StreamingModularDrm extends DrmHandler {

    byte[] executeKeyRequest(ModularDrmKeyRequest request) throws DrmRequestException;
+
+    final class DrmRequestException extends Exception {


Only used for StreamingModularDrm so inlined on the interface.

Mecharyry · 2017-06-28T14:58:12Z

core/src/main/java/com/novoda/noplayer/internal/Heart.java

@@ -63,24 +65,19 @@ public boolean isBeating() {
        return beating;
    }

-    public static class Heartbeat<T> implements Runnable {


No need for the generic, we always pass back a Player.

Mecharyry · 2017-06-28T14:59:01Z

core/src/main/java/com/novoda/noplayer/internal/exoplayer/forwarder/EventInfoForwarder.java


 import java.util.HashMap;

 class EventInfoForwarder implements ExoPlayer.EventListener {

-    private final InfoListeners infoListeners;
+    private final Player.InfoListener infoListener;


Coding to the exposed interface rather than the internal class.

Mecharyry · 2017-06-28T15:00:27Z

core/src/main/java/com/novoda/noplayer/internal/listeners/PlayerListenersHolder.java

@@ -125,39 +119,39 @@ public void removeVideoSizeChangedListener(Player.VideoSizeChangedListener video
        videoSizeChangedListeners.remove(videoSizeChangedListener);
    }

-    public ErrorListeners getErrorListeners() {
+    public Player.ErrorListener getErrorListeners() {


Use the interfaces, so that we can make the ErrorListeners etc package protected.

jszmltr · 2017-06-28T15:57:13Z

core/src/main/java/com/novoda/noplayer/internal/exoplayer/mediasource/TrackType.java

@@ -0,0 +1,7 @@
+package com.novoda.noplayer.internal.exoplayer.mediasource;
+
+enum TrackType {


Where is this coming from ?

Just moved from exoplayer.mediasource to internal.exoplayer.mediasource.

jszmltr · 2017-06-28T15:58:19Z

core/src/main/java/com/novoda/noplayer/internal/mediaplayer/AndroidMediaPlayerImpl.java

@@ -180,7 +180,7 @@ public void seekTo(VideoPosition position) throws IllegalStateException {
    }

    @Override
-    public void pause() {  // TODO: throw to match ExoPlayer?
+    public void pause() throws IllegalStateException {


jszmltr · 2017-06-28T15:58:44Z

...rc/main/java/com/novoda/noplayer/internal/mediaplayer/forwarder/BufferHeartbeatListener.java

+import com.novoda.noplayer.Player;
+import com.novoda.noplayer.internal.mediaplayer.CheckBufferHeartbeatCallback;
+
+class BufferHeartbeatListener implements CheckBufferHeartbeatCallback.BufferListener {


Is this moved from somewhere else as well ?

Yes, this was in the mediaplayer.forwarder package before. Now it's been moved to internal.mediaplayer.forwarder.

Ryan Feline added 6 commits June 28, 2017 13:56

Move exoplayer and mediaplayer packages to internal.

2084000

Use interface for all Listeners.

0c21c8c

Move listeners to internal package.

638fb6e

Drop PrioritisedPlayers.

f8c230e

Move Drm provision classes to internal package.

0761108

Collapse player package.

28adbf9

Mecharyry added enhancement housekeeping labels Jun 28, 2017

Mecharyry changed the base branch from master to exo-player-2 June 28, 2017 14:30

juankysoriano self-requested a review June 28, 2017 14:31

juankysoriano self-assigned this Jun 28, 2017

juankysoriano reviewed Jun 28, 2017

View reviewed changes

juankysoriano approved these changes Jun 28, 2017

View reviewed changes

Ryan Feline added 2 commits June 28, 2017 15:43

Make the HeartBeat callback consistent with others by moving to Playe…

9d4e414

…r interface.

Merge branch 'exo-player-2' into ALL-3936/internal

c64c923

jszmltr self-assigned this Jun 28, 2017

jszmltr self-requested a review June 28, 2017 14:51

Mecharyry commented Jun 28, 2017

View reviewed changes

jszmltr reviewed Jun 28, 2017

View reviewed changes

jszmltr merged commit 898c024 into exo-player-2 Jun 29, 2017

jszmltr deleted the ALL-3936/internal branch June 29, 2017 07:25

ouchadam mentioned this pull request Jul 5, 2017

ExoPlayer2 / Major library redesign #68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ALL-3936/Internal package #59

ALL-3936/Internal package #59

Mecharyry commented Jun 28, 2017 •

edited

Loading

juankysoriano Jun 28, 2017

ouchadam Jun 28, 2017

juankysoriano left a comment

Mecharyry Jun 28, 2017

Mecharyry Jun 28, 2017

Mecharyry Jun 28, 2017

Mecharyry Jun 28, 2017

Mecharyry Jun 28, 2017

Mecharyry Jun 28, 2017

Mecharyry Jun 28, 2017

jszmltr Jun 28, 2017

Mecharyry Jun 28, 2017

jszmltr Jun 28, 2017

jszmltr Jun 28, 2017

Mecharyry Jun 28, 2017

		@@ -0,0 +1,7 @@
		package com.novoda.noplayer.internal.exoplayer.mediasource;

		enum TrackType {

ALL-3936/Internal package #59

ALL-3936/Internal package #59

Conversation

Mecharyry commented Jun 28, 2017 • edited Loading

Problem

Solution

Test(s) added

Screenshots

Paired with

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juankysoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mecharyry commented Jun 28, 2017 •

edited

Loading