Feature play with button #571

okan35 · 2020-09-24T18:34:50Z

Feature to choose player from the menu where we have play button. I created this because some files play fine on vlc and some play fine on external app. It is better to have such button than going to settings and changing there.

Changes
Added play with button next to play button. This button will only be visible if preferred player is set external app.

Issues
#561

nielsvanvelzen

Thanks for your PR. I did an initial review on the changed code and would like to request some changes to your implementation:

Right now the playWithButton is added when the preferred player is "External". This should be changed to a new player type called "Choose" (string: "Always ask")
In adition, the playWithButton should replace the play button
When a player is chosen for playback the user preferences should not be changed, instead only the next playing file should use the chosen player.

nielsvanvelzen · 2020-09-25T06:43:58Z

app/src/main/res/menu/menu_details_play_with.xml

+<menu
+    xmlns:android="http://schemas.android.com/apk/res/android">
+
+    <item android:title="Play with Vlc"


All titles should use strings so they can be translated.

nielsvanvelzen · 2020-09-25T06:44:40Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

@@ -1260,6 +1267,19 @@ public void onClick(View v) {
                more.show();
            }
        });
+        if (userPreferences.getValue().get(UserPreferences.Companion.getVideoPlayer()) == PreferredVideoPlayer.EXTERNAL) {
+            playWithButton = new TextUnderButton(this, R.drawable.ic_add, buttonSize, 3, "Play with", new View.OnClickListener() {


"Play with" should use a string so it can be translated.

nielsvanvelzen · 2020-09-25T06:45:28Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

+            playWithButton = new TextUnderButton(this, R.drawable.ic_add, buttonSize, 3, "Play with", new View.OnClickListener() {
+                @Override
+                public void onClick(View view) {
+                    Toast.makeText(mApplication, "Play With Button", Toast.LENGTH_SHORT).show();


This toast looks like it was intended for debugigng purposes, should probably remove it or replace it with a logger debug message.

nielsvanvelzen · 2020-09-25T06:46:35Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

+            switch (item.getItemId()) {
+
+                case R.id.play_with_vlc:
+                        userPreferences.getValue().set(UserPreferences.Companion.getVideoPlayer(), PreferredVideoPlayer.VLC);


User preferences should never be changed without the user explicitly telling the application to do so. In this case you should find a way to set the player for the next play call without changing the setting.

nielsvanvelzen

Some files have weird formatting or too many line breaks. Please check those and fix where necessary, but try to avoid reformatting the whole file.

nielsvanvelzen · 2020-09-26T18:45:33Z

app/src/main/java/org/jellyfin/androidtv/preference/UserPreferences.kt

+		 * chosen player for play with button - changes every time user chooses a player with "play with" button
+		 */
+
+		var chosenPlayer = Preference.enum("chosen_player",PreferredVideoPlayer.VLC)


Should use SystemPreferences

nielsvanvelzen · 2020-09-26T18:46:51Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

            if (resumeButtonVisible) {
                mResumeButton.requestFocus();
            } else {
-                play.requestFocus();
+                if (!isPlayerPrefChoose) {


Use else if

nielsvanvelzen · 2020-09-26T18:48:36Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

@@ -1260,6 +1276,18 @@ public void onClick(View v) {
                more.show();
            }
        });
+        if (userPreferences.getValue().get(UserPreferences.Companion.getVideoPlayer()) == PreferredVideoPlayer.CHOOSE) {


Probably easier to add an else statement when checking if the play button should be shown

nielsvanvelzen · 2020-09-26T18:48:58Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

@@ -1260,6 +1276,18 @@ public void onClick(View v) {
                more.show();
            }
        });
+        if (userPreferences.getValue().get(UserPreferences.Companion.getVideoPlayer()) == PreferredVideoPlayer.CHOOSE) {
+            playWithButton = new TextUnderButton(this, R.drawable.ic_add, buttonSize, 3, getString(R.string.play_with), new View.OnClickListener() {


Since either the play or play with button is shown you can probably reuse the playButton property

nielsvanvelzen · 2020-09-26T18:51:23Z

app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackController.java

+                                if (preferredVideoPlayerByPlayWith == PreferredVideoPlayer.VLC) {
+                                    useVlc = true;
+                                } else if (preferredVideoPlayerByPlayWith == PreferredVideoPlayer.EXOPLAYER) {
+                                    // Make sure to not use VLC
+                                    useVlc = false;
+                                }


Suggested change

if (preferredVideoPlayerByPlayWith == PreferredVideoPlayer.VLC) {

useVlc = true;

} else if (preferredVideoPlayerByPlayWith == PreferredVideoPlayer.EXOPLAYER) {

// Make sure to not use VLC

useVlc = false;

}

useVlc = preferredVideoPlayerByPlayWith == PreferredVideoPlayer.VLC;

nielsvanvelzen · 2020-09-26T18:52:15Z

app/src/main/res/values/strings.xml

@@ -403,4 +404,8 @@
    <string name="not_deleted">Item NOT Deleted</string>
    <string name="turn_off">Turn this option off</string>
    <string name="just_one">Just this one</string>
+    <string name="play_with_vlc">Play with Vlc</string>
+    <string name="play_with_exo_player">Play with Exo Player</string>


Suggested change

<string name="play_with_exo_player">Play with Exo Player</string>

<string name="play_with_exo_player">Play with ExoPlayer</string>

nielsvanvelzen · 2020-09-26T18:52:30Z

app/src/main/res/values/strings.xml

@@ -403,4 +404,8 @@
    <string name="not_deleted">Item NOT Deleted</string>
    <string name="turn_off">Turn this option off</string>
    <string name="just_one">Just this one</string>
+    <string name="play_with_vlc">Play with Vlc</string>
+    <string name="play_with_exo_player">Play with Exo Player</string>
+    <string name="play_with_external_app">Play with External App</string>


Suggested change

<string name="play_with_external_app">Play with External App</string>

<string name="play_with_external_app">Play with external app</string>

nielsvanvelzen · 2020-09-26T18:54:15Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

@@ -1325,6 +1353,42 @@ public boolean onMenuItemClick(MenuItem item) {
            return false;
        }
    };
+    PlaybackController playbackController= new  PlaybackController(MediaManager.getCurrentVideoQueue());


I think this is unused

nielsvanvelzen · 2020-09-26T18:54:23Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

@@ -1325,6 +1353,42 @@ public boolean onMenuItemClick(MenuItem item) {
            return false;
        }
    };
+    PlaybackController playbackController= new  PlaybackController(MediaManager.getCurrentVideoQueue());
+
+    PopupMenu.OnMenuItemClickListener playWithMenuListener = new PopupMenu.OnMenuItemClickListener() {


Mark private

nielsvanvelzen · 2020-09-26T18:54:35Z

app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackController.java

@@ -112,6 +112,19 @@ public PlaybackController(List<BaseItemDto> items, IPlaybackOverlayFragment frag
        useVlc = userPreferences.getValue().get(UserPreferences.Companion.getVideoPlayer()) == PreferredVideoPlayer.VLC;
    }

+    public PlaybackController(List<BaseItemDto> items) {


I think this is unused too

nielsvanvelzen

The "choose" option shows up for LiveTV. But I don't have LiveTV setup so I can't test if that works
The "choose" button shows up on the artist page but music shouldn't do that

nielsvanvelzen · 2020-10-04T09:58:08Z

app/src/main/java/org/jellyfin/androidtv/preference/SystemPreferences.kt

@@ -59,5 +60,11 @@ class SystemPreferences(context: Context) : SharedPreferenceStore(
 		 * Stores whether the sports filter is active in the channel guide or not
 		 */
 		val liveTvGuideFilterSports = Preference.boolean("guide_filter_sports", false)
+
+		/**
+		 * chosen player for play with button - changes every time user chooses a player with "play with" button


Suggested change

* chosen player for play with button - changes every time user chooses a player with "play with" button

* Chosen player for play with button - changes every time user chooses a player with "play with" button.

nielsvanvelzen · 2020-10-04T09:59:31Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

-            TextUnderButton play = new TextUnderButton(this, R.drawable.ic_play, buttonSize, 2, getString(BaseItemUtils.isLiveTv(mBaseItem) ? R.string.lbl_tune_to_channel : mBaseItem.getIsFolderItem() ? R.string.lbl_play_all : R.string.lbl_play), new View.OnClickListener() {
+        //playButton becomes playWith button
+        if (userPreferences.getValue().get(UserPreferences.Companion.getVideoPlayer()) == PreferredVideoPlayer.CHOOSE) {
+            playButton = new TextUnderButton(this, R.drawable.ic_add, buttonSize, 3, getString(R.string.play_with), new View.OnClickListener() {


Suggested change

playButton = new TextUnderButton(this, R.drawable.ic_add, buttonSize, 3, getString(R.string.play_with), new View.OnClickListener() {

playButton = new TextUnderButton(this, R.drawable.ic_play, buttonSize, 3, getString(R.string.play_with), new View.OnClickListener() {

This one isn't fixed yet. We should use the same icon.

okan35 · 2020-10-04T10:53:50Z

I haven't even seen anything related to music, So I have to check this. I also don't listen to music on jellyfin either but I believe if the page is music we don't show play with button, right ?

nielsvanvelzen · 2020-10-04T10:55:28Z

The FullDetailsActivity is shared for pretty much everything that exists and the play with button did show up for an artist for me.

okan35 · 2020-10-04T11:08:55Z

So basically if the current page is for music, we don't show play with button, right ?

nielsvanvelzen · 2020-10-04T12:00:00Z

It happens now but it shouldn't

nielsvanvelzen · 2020-10-18T14:11:49Z

I just tested it and it doesn't work for episodes

okan35 · 2020-10-18T17:01:22Z

I will check it now, try on my mibox what exactly is not working ?

nielsvanvelzen · 2020-10-18T17:12:51Z

The normal play button showed up in the episode page instead of the play with button

jellyfin-bot · 2020-10-18T17:32:01Z

Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

Complexity increasing per file
==============================
- app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java  5

See the complete overview on Codacy

jellyfin-bot · 2020-10-18T17:32:02Z

app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java

@@ -839,6 +843,7 @@ public void onClick(DialogInterface dialog, int which) {
    private TextUnderButton queueButton = null;
    private TextUnderButton deleteButton = null;
    private TextUnderButton moreButton;
+    private TextUnderButton playButton = null;


Codacy found an issue: Fields should be declared at the top of the class, before any method declarations, constructors, initializers or inner classes.

okan35 added 2 commits September 21, 2020 21:16

play with work

4612559

menu is working

219545b

nielsvanvelzen requested changes Sep 25, 2020

View reviewed changes

okan35 added 2 commits September 26, 2020 14:21

progress on play with feature

502c3ff

working external player

d4f5774

nielsvanvelzen requested changes Sep 26, 2020

View reviewed changes

nielsvanvelzen added this to In progress in v0.12.0 via automation Sep 26, 2020

okan35 added 2 commits September 27, 2020 11:19

changes from review

8ffba55

removed unused imports - string change

908f99c

nielsvanvelzen requested changes Oct 4, 2020

View reviewed changes

okan35 added 2 commits October 5, 2020 20:57

show playWith button on certain screens

401799c

button icon change

893a3bf

jellyfin-bot reviewed Oct 18, 2020

View reviewed changes

episode check added for play with button

ea11eaa

nielsvanvelzen approved these changes Oct 18, 2020

View reviewed changes

nielsvanvelzen merged commit 2e930f9 into jellyfin:master Oct 18, 2020

v0.12.0 automation moved this from In progress to Done Oct 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature play with button #571

Feature play with button #571

okan35 commented Sep 24, 2020

nielsvanvelzen left a comment

nielsvanvelzen Sep 25, 2020

nielsvanvelzen Sep 25, 2020

nielsvanvelzen Sep 25, 2020

nielsvanvelzen Sep 25, 2020

nielsvanvelzen left a comment

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen Sep 26, 2020

nielsvanvelzen left a comment

nielsvanvelzen Oct 4, 2020

nielsvanvelzen Oct 4, 2020

nielsvanvelzen Oct 10, 2020

okan35 commented Oct 4, 2020

nielsvanvelzen commented Oct 4, 2020

okan35 commented Oct 4, 2020

nielsvanvelzen commented Oct 4, 2020

nielsvanvelzen commented Oct 18, 2020

okan35 commented Oct 18, 2020 •

edited

nielsvanvelzen commented Oct 18, 2020

jellyfin-bot commented Oct 18, 2020

jellyfin-bot Oct 18, 2020

	<string name="play_with_exo_player">Play with Exo Player</string>
	<string name="play_with_exo_player">Play with ExoPlayer</string>

	<string name="play_with_external_app">Play with External App</string>
	<string name="play_with_external_app">Play with external app</string>

	* chosen player for play with button - changes every time user chooses a player with "play with" button
	* Chosen player for play with button - changes every time user chooses a player with "play with" button.

	playButton = new TextUnderButton(this, R.drawable.ic_add, buttonSize, 3, getString(R.string.play_with), new View.OnClickListener() {
	playButton = new TextUnderButton(this, R.drawable.ic_play, buttonSize, 3, getString(R.string.play_with), new View.OnClickListener() {

Feature play with button #571

Feature play with button #571

Conversation

okan35 commented Sep 24, 2020

nielsvanvelzen 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

nielsvanvelzen 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

nielsvanvelzen 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

okan35 commented Oct 4, 2020

nielsvanvelzen commented Oct 4, 2020

okan35 commented Oct 4, 2020

nielsvanvelzen commented Oct 4, 2020

nielsvanvelzen commented Oct 18, 2020

okan35 commented Oct 18, 2020 • edited

nielsvanvelzen commented Oct 18, 2020

jellyfin-bot commented Oct 18, 2020

Choose a reason for hiding this comment

okan35 commented Oct 18, 2020 •

edited