Add still watching feature #4509
Conversation
|
I have this like 80% done I would say, but I have hit a roadblock I could use some help with. I have the correct logic in place that will display the are you still watching screen in the correct scenario, but I can't get the actual fragment for this screen to appear. Changing the |
ConnorS1110
force-pushed
the
still-watching
branch
from
99ad911 to
38ba082
Compare
|
This feature might not be accepted as I think it would be preferred for this to be in the jellyfin server. So that it can be implemented by all the different client devices |
|
What part are you envisioning is at the server level? The logic to decide if the still watching screen should be displayed? There still would need to be changes in each of the consuming clients to display the client specific implementation of the screen and client specific implementations to notify the server if there has been any any device specific inputs to reset the counter. |
Yes the logic should be decided on the server once that is implemented it can be adopted for different devices/apps to give a consistent system. @nielsvanvelzen would probably be the guy to answer this. But as far as I can tell device specific features are unlikely to get implemented. |
|
@Dnkhatri I don't think that's right - this specific feature resets the timer every time the android tv remote is used, which is not an event the server will ever be aware of. This feature definitely should be implemented on the client. |
|
We have discussed it on element/discord and the consensus is that there is no reason for this to be server side at all, except for user config settings to customize the conditions to trigger the screen. So this will largely stay as is |
|
@johnpc I hadn't seen that PR thanks for sharing. It does look like it hasn't been worked on in 2 months though, idk if the contributor is still working on it and just hasn't pushed their changes or what. That PR does look like it is doing what I was hoping to accomplish. I am still not done with this one, I am having issues with fragment management to actually get the screen to appear. From my testing, it does still properly track the condition to display the still watching screen, but displaying the fragment itself is proving to be more troublesome than I thought. If you experienced other issues I would be interested to know what those issues were |
ConnorS1110
force-pushed
the
still-watching
branch
2 times, most recently
from
a288436 to
428aa96
Compare
|
A lot of work was done here. This was based off the work @kinhelm did in PR #4389. If/when this merges, they should also get credit for it as well. I did add 2 temp settings for this feature to make it easier to test. 1 to test the episode count condition, and 1 to test the min watch time condition. |
It's beautiful! |
| val stillWatchingSetting = StillWatchingStates.getSetting(userPreferences[UserPreferences.stillWatchingEnabled].toString()) | ||
|
|
||
| val currentEpisodeProgress = videoManager.currentPosition.toFloat() / videoManager.duration.toFloat() | ||
| val minMinutesInMs = stillWatchingSetting.minMinutes * MILLIS_PER_MIN |
There was a problem hiding this comment.
| val minMinutesInMs = stillWatchingSetting.minMinutes * MILLIS_PER_MIN | |
| val minMinutesInMs = stillWatchingSetting.minMinutes.toLong() * MILLIS_PER_MIN |
There was a problem hiding this comment.
I had to make this change in order for the build to succeed on my machine.
There was a problem hiding this comment.
Huh I see android studio is giving me an error on that line as well. Idk how it built before but it will be fixed in my next push
| context: Context, | ||
| attrs: AttributeSet? = null | ||
| ) : FrameLayout(context, attrs) { | ||
| private var countdownTimer: CountDownTimer? = null |
There was a problem hiding this comment.
How long does the countdown timer last by default in other in other platforms like Netflix? It was jarring when it returned me to the Home Screen - I would have rather let it just sit there forever on "Are you still watching?". I could imagine myself falling asleep watching TV, then waking up and clicking "yep continue actually" even though it would have been paused for a long time by the time I wake up.
In that case I suppose I would be suggesting to remove the count down timer altogether. This is just my own design preference though so if the standards and expectations in other apps prefer a countdown, it's probably better to stick with whatever conforms best to user expectations.
There was a problem hiding this comment.
I will just remove it and if there is a feature request for it later, I can add it back. Should be straightforward to re-add if needed.
|
One other thing I noticed - the default focus is on "No". I don't think that makes sense. The default focus should be on "Yes" so if I grab my remote and tap it, the show continues. Right now if I grab the remote and tap it, it closes the player. |
|
Another thing I noticed - if I choose "Yes" once, it never prompts again. I would expect that after the time elapses again, the popup would reappear. |
| @@ -161,6 +164,8 @@ public void onCreate(Bundle savedInstanceState) { | |||
|
|
|||
| playbackControllerContainer.getValue().setPlaybackController(new PlaybackController(mItemsToPlay, this, mediaPosition)); | |||
|
|
|||
| playbackController = playbackControllerContainer.getValue().getPlaybackController(); | |||
There was a problem hiding this comment.
This is well intentioned but it makes the diff a lot harder to read. Is there any change you intended to make to this file?
There was a problem hiding this comment.
As I was working on this it looked like I was going to need to make changes to this file, and as I was working in it, I realized there was A LOT of duplicate code to get the playbackController, so I just refactored it. It turns out I didn't need to touch this file at all though. I think I would still advocate for keeping the refactor, even though the changes are no longer relevant to the feature, since it changes no logic and just makes the file more readable. For review purposes it could be largely ignored. It would be easy to revert if there was an issue. I'll leave it up to you if these changes should stay or not.
| @@ -135,9 +135,12 @@ public class CustomPlaybackOverlayFragment extends Fragment implements LiveTvGui | |||
| private final Lazy<NavigationRepository> navigationRepository = inject(NavigationRepository.class); | |||
| private final Lazy<BackgroundService> backgroundService = inject(BackgroundService.class); | |||
| private final Lazy<ImageHelper> imageHelper = inject(ImageHelper.class); | |||
| private final Lazy<WatchTrackerViewModel> watchTracker = inject(WatchTrackerViewModel.class); | |||
There was a problem hiding this comment.
It doesn't look like this is used, is it?
There was a problem hiding this comment.
You're right it will be removed in my next push
| @@ -168,6 +170,10 @@ class MainActivity : FragmentActivity() { | |||
| super.onUserInteraction() | |||
|
|
|||
| screensaverViewModel.notifyInteraction(false) | |||
|
|
|||
| if (watchTracker.getItemIsEpisode()) { | |||
| watchTracker.onUserInteraction() | |||
There was a problem hiding this comment.
I think I'd rather see watchTracker.notifyInteraction() and then the implementation within watchTracker would check if it's an episode or not.
There was a problem hiding this comment.
Will be changed in next push
| @@ -481,6 +487,8 @@ public void onClick(DialogInterface dialog, int which) { | |||
| mFragment.setPlayPauseActionState(0); | |||
| mFragment.setCurrentTime(position); | |||
|
|
|||
| if (watchTracker.getItemIsEpisode() && !watchTracker.getItemWasInterrupted()) watchTracker.updateLastUpdateTime(); | |||
There was a problem hiding this comment.
same comment as above, I think I prefer watchTracker.notifyPlay() and let the watchTracker be in charge of isEpisode/interupted logic.
There was a problem hiding this comment.
Will be changed in next push
| @@ -1179,6 +1190,8 @@ public void onProgress() { | |||
| stopSpinner(); | |||
| } | |||
| } | |||
|
|
|||
| if (watchTracker.getItemIsEpisode() && !watchTracker.getItemWasInterrupted()) watchTracker.updateWatchTime(); | |||
There was a problem hiding this comment.
same comment here with like watchTracker.notifyProgress()
There was a problem hiding this comment.
Will be changed in next push
| val minMinutesInMs = stillWatchingSetting.minMinutes * MILLIS_PER_MIN | ||
|
|
||
| // At episode count, your watch time is above min minute threshold, and you are at least 10% of the way through the next episode | ||
| val episodeRequirementMet = episodeCount == stillWatchingSetting.episodeCount && watchTime >= minMinutesInMs && currentEpisodeProgress >= 0.1 |
There was a problem hiding this comment.
why does episodeRequirementMet include watch time? Isn't watch time supposed to be considered in the next line?
| // At episode count, your watch time is above min minute threshold, and you are at least 10% of the way through the next episode | ||
| val episodeRequirementMet = episodeCount == stillWatchingSetting.episodeCount && watchTime >= minMinutesInMs && currentEpisodeProgress >= 0.1 | ||
|
|
||
| // Above min minute threshold and you are not on the episode to meet episode requirement |
There was a problem hiding this comment.
Can you explain and you are not on the episode to meet episode requirement? This feels superfluous
|
|
||
| override fun onFinish() { | ||
| // Perform a click so the event handler will activate | ||
| view.fragmentStillWatchingButtonsNo.performClick() |
There was a problem hiding this comment.
This reminds me - do other parts of the codebase call .performClick() in a fashion that would cause a user action event? For example, on the NextUpButtonsView.kt calls .performClick() and I'm concerned that might be treated as a user interaction that resets the Still Watching timer/episode count.
There was a problem hiding this comment.
So to address this, it is going to run counter to what you suggested earlier, re: Having the viewmodel be completely in charge of deciding whether there was an interaction/it should start/etc. I need to ignore inputs when the still watching fragment is open, so I am not resetting things when I shouldn't be. I need the fragmentManager to do that, and thats a big ViewModel no-no since the ViewModel shouldn't be handling UI logic. In the activity, I am going to still need to keep the interaction function wrapped in an if statement so I can properly determine if the fragment is visible or not
There was a problem hiding this comment.
I need to ignore inputs when the still watching fragment is open
It might be a bit fragile if other .performClick()s get added in the future since you'll have to work around those in order to keep the Still Watching feature behaving as expected.
| _binding = FragmentStillWatchingBinding.inflate(inflater, container, false) | ||
|
|
||
| binding.fragmentStillWatchingButtons.apply { | ||
| // duration = userPreferences[UserPreferences.nextUpTimeout] |
There was a problem hiding this comment.
If we must have a countdown timer for this (I really don't like the countdown timer the more I think about it), then we should definitely have a user preference instead of hardcoding 30 seconds (which is actually super duper fast unless your remote is taped to your hands).
There was a problem hiding this comment.
I am getting rid of the timer
| return when (type.lowercase()) { | ||
| "test_episode_count" -> StillWatchingStates(enabled = true, episodeCount = 2, minMinutes = 1) | ||
| "test_min_minutes" -> StillWatchingStates(enabled = true, episodeCount = 5, minMinutes = 0.5) | ||
| "short" -> StillWatchingStates(enabled = true, episodeCount = 2, minMinutes = 60) |
There was a problem hiding this comment.
How does this affect long shows with >60m episodes?
ConnorS1110
force-pushed
the
still-watching
branch
from
428aa96 to
94979fb
Compare
|
@johnpc Pushed again and addressed everything to the best of my ability. Now I will explain the logic behind the 2 requirements. Episode Requirement: We need to ensure we are above the min minute threshold in order to say we have met the episode requirement. Imagine we are watching episodes that are 10 minutes long, Within 30 minutes, we would hit the condition for the default setting to show the screen, even though the min number of minutes is 90. So if we had normal to longer episodes (45 minutes - 1 hr +), this condition will likely get tripped Time Requirement: There needs to be another condition that handles if you are watching a lot of short episodes. That is what this is for. The logic has been changed so that watchTime must be greater than or equal to the min minute threshold of your user setting AND your episode count must be greater than the number from your user setting. This ensures we capture short media to still display the screen at an appropriate interval. Hope this makes it make more sense |
| fun onEpisodeWatched() { | ||
| Timber.i("Watcher onEpisodeWatched") | ||
| if (!itemWasInterrupted) episodeCount++ | ||
| itemWasInterrupted = false |
There was a problem hiding this comment.
Also, this seems to affect whether I touch the remote at all during an episode. So if I touch the remote at the beginning of an episode (to set subtitles or see how long it is or something), it essentially doesn't count toward the still watching feature
| val currentEpisodeProgress = videoManager.currentPosition.toFloat() / videoManager.duration.toFloat() | ||
| val minMinutesInMs = stillWatchingSetting.minMinutes * MILLIS_PER_MIN | ||
|
|
||
| // At episode count, your watch time is above min minute threshold, and you are at least 10% of the way through the next episode |
There was a problem hiding this comment.
I saw your explanation
Episode Requirement: We need to ensure we are above the min minute threshold in order to say we have met the episode requirement. Imagine we are watching episodes that are 10 minutes long, Within 30 minutes, we would hit the condition for the default setting to show the screen, even though the min number of minutes is 90. So if we had normal to longer episodes (45 minutes - 1 hr +), this condition will likely get tripped
The code for this is
val episodeRequirementMet = episodeCount == stillWatchingSetting.episodeCount - 1 && watchTime >= minMinutesInMs && currentEpisodeProgress >= 0.1
The 10% of currentEpisodeProgress still throws me off. and I don't think the episodeCount == stillWatchingSetting.episodeCount - 1 makes sense either. Can you clarify what we're going for as it related to the code?
| val episodeRequirementMet = episodeCount == stillWatchingSetting.episodeCount - 1 && watchTime >= minMinutesInMs && currentEpisodeProgress >= 0.1 | ||
|
|
||
| // Above min minute threshold and you have watched more episodes than is required | ||
| val watchTimeRequirementMet = watchTime >= minMinutesInMs && episodeCount > stillWatchingSetting.episodeCount |
There was a problem hiding this comment.
Same comment here. I see you description
Time Requirement: There needs to be another condition that handles if you are watching a lot of short episodes. That is what this is for. The logic has been changed so that watchTime must be greater than or equal to the min minute threshold of your user setting AND your episode count must be greater than the number from your user setting. This ensures we capture short media to still display the screen at an appropriate interval.
I'm having a little trouble wrapping my head around it though. In my original mental model, the watch time and episode count logic were independent from one another. Now I see that they are dependent, but it's tricky for me to understand how exactly they interact.
I also worry this could cause customer confusion. Like "I set my 'Still Watching' time setting to be 60 seconds but it's not firing, is it broken?" I think the rule that governs triggering the popup should be so simple it can be explained in a sentence without triggering lots of other questions.
|
Also just a reminder, my suggestions come from an interest in your feature, a love of jellyfin and a desire to build great software, but I am not a maintainer just a guy so that's all they are is suggestions. :D |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| private fun itemIsEpisode(item: BaseItemDto? = null): Boolean { | ||
| val newItem = item |
Check warning
Code scanning / detekt
Property is unused and should be removed.
There was a problem hiding this comment.
is ktlintFormat set up for this project? If so you might just have to run that.
| val minMinutesInMs = stillWatchingSetting.minMinutes.toLong() * MILLIS_PER_MIN | ||
|
|
||
| // At episode count, your watch time is above min minute threshold, and you are at least 10% of the way through the next episode | ||
| val episodeRequirementMet = episodeCount == stillWatchingSetting.episodeCount - 1 && watchTime >= minMinutesInMs && currentEpisodeProgress >= 0.1 |
Check warning
Code scanning / detekt
Line detected, which is longer than the defined maximum line length in the code style.
| val minMinutesInMs = stillWatchingSetting.minMinutes.toLong() * MILLIS_PER_MIN | ||
|
|
||
| // At episode count, your watch time is above min minute threshold, and you are at least 10% of the way through the next episode | ||
| val episodeRequirementMet = episodeCount == stillWatchingSetting.episodeCount - 1 && watchTime >= minMinutesInMs && currentEpisodeProgress >= 0.1 |
Check warning
Code scanning / detekt
Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers.
| @@ -281,6 +284,10 @@ | |||
| normalHeight = mExoPlayerView.getLayoutParams().height; | |||
| } | |||
|
|
|||
| public FragmentActivity getActivity() { | |||
Check notice
Code scanning / Android Lint
Unknown nullness
| xmlns:app="http://schemas.android.com/apk/res-auto" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" | ||
| android:background="@color/black_transparent"> |
Check warning
Code scanning / Android Lint
Overdraw: Painting regions more than once
ConnorS1110
force-pushed
the
still-watching
branch
from
94979fb to
3859afb
Compare
|
At this point, I am going to wait for a maintainer to give thoughts on my implementation before working on this more. Hopefully this can get finalized soon and I can start working on making a matching implementation for web |
There was a problem hiding this comment.
We already have interaction tracking for our screensaver so I'd rather add a new "InteractionTracker" that will be used by both the screensaver and this feature.
We will not be adding popups during video playback, that's an anti-pattern. Instead, this feature should be implemented in the next-up screen: when a video item ends and we want to know if a user is still watching it will show a different screen that will behave similarly to the popup you currently have.
All new UI must be written in Compose.
|
@nielsvanvelzen almost done with the changes you requested, but a design question. Thoughts on the screen looking nearly identical to the "Next Up" screen, except the words "Are you still watching?" along with the buttons are in the center of the screen? I thought about still having the next up info in the bottom right corner, just minus the buttons. Or did you envision this screen would look literally EXACTLY the same as the Next Up screen, just with the verbiage describing the next episode removed with just the "Are you still watching?" question with action buttons? Edit: I did just push up everything. Only thing left is just finalizing the visual design, but the buttons work, the screen displays only at the end of an episode, I combined the viewmodel for the screensaver and this feature, updated all the references for both features, and the user settings still work |
ConnorS1110
force-pushed
the
still-watching
branch
from
3859afb to
c682a55
Compare
ConnorS1110
force-pushed
the
still-watching
branch
3 times, most recently
from
e24bb00 to
ce7b31d
Compare
ConnorS1110
force-pushed
the
still-watching
branch
from
ce7b31d to
c850c59
Compare
|
Alright I think I have finalized what was asked to be changed and I personally think the design I have looks pretty good. Going to say ready for a formal re-review @nielsvanvelzen |
Changes
Monitor how many episodes have been watched in a row with no remote interaction. Default behavior is if 3 episodes have been watched, or there was 90 minutes of watch time, with no interaction, a timer starts when watching the 4th episode. If timer runs out with no interaction, "Are you still watching prompt" appears. Another countdown starts and a progress bar fills the "No" button. When countdown ends, and a choice hasn't been selected playback ends. There are a series of user customizable options to configure the conditions the screen will trigger, or you can disable it all together.
Issues
Adds a feature that has been requested for 5 years now
Also requested in #1327