Sleep Timer (new attempt) #12557
Sleep Timer (new attempt) #12557
Conversation
|
I added a new sleep timer after the PR #2892 got closed. This is not based on the other PR, but I tried to address the issues with the other PR. Some notes / open questions:
|
TobiGr
added
feature request
There was a problem hiding this comment.
Hello and thank you for looking into this!
Note: @Stypox is the player master, he knows the player better than me. So take my comments with a grain of salt. You might want to wait with implementing the suggestion before he commented on this.
I added the StringArray
sleep_timer_descriptionand the IntArraysleep_timer_valuetosettings_keys.xml. This does not seem to be the right place, but I added them there becauseseek_duration_descriptionandseek_duration_valuelive there. Should I create a new XML-file (e.g.menu_items.xml)?
Sounds like a good idea in general. However, I am not sure if we should have a fixed list of options. I'd prefer to have a fragment which allows me to enter a custom value. That might be done at a later point though.
When the timer runs out, the
PlayercallsplayerUi.onSleepTimerUpdatein the timer-Thread. The UI then usesbinding.sleepTimerTextView.postto switch to the UI thread, to callplayer.pause. It seems a bit convoluted to me, but it is the cleanest way I could think of. Is there a better way to do this?
You could use a Disposable like this to run on the correct thread. You might want to check whether .timer().repeat() or .interval() should be used, I guess the latter.
@NonNull
private final CompositeDisposable sleepTimerDisposable = new CompositeDisposable();
// [...]
public void saveAndShutDown() {
// [...]
cancelSleepTimer();
// [...]
}
// [...]
public void setSleepTimer(@Nullable final long sleepMinutes) {
sleepTimerEnd = java.time.Instant.now().plus(sleepMinutes, ChronoUnit.MINUTES);
final Player thisPlayer = this;
final Disposable sleepTimer = Observable.timer(1, TimeUnit.SECONDS)
.repeat()
.observeOn(AndroidSchedulers.mainThread())
.subscribe(t -> {
if (java.time.Instant.now().isAfter(sleepTimerEnd)) {
UIs.call(playerUi -> playerUi.onSleepTimerUpdate(0));
thisPlayer.pause();
cancelSleepTimer();
} else {
final long remainingMinutes = java.time.Instant.now()
.until(sleepTimerEnd, ChronoUnit.MINUTES);
UIs.call(ui -> ui.onSleepTimerUpdate(remainingMinutes + 1));
}
}, throwable -> {
// FIXME: Use proper error handling here, e.g. ErrorUtil
if (DEBUG) {
Log.e(TAG, "Sleep timer error", throwable);
}
});
sleepTimerDisposable.clear();
sleepTimerDisposable.add(sleepTimer);
}
public void cancelSleepTimer() {
sleepTimerDisposable.clear(); // or .dispose() please check which one should be used here
}On a different note: we also have a background player (see PlayQueueActivity) which would make best use of this feature. The sleep timer for videos is good for TV users but the I assume the sleep timer in the background player would be used by most people. It would be good to also add a way to interact with the timer there.
| <string name="sleep_timer_popup_title">Sleep Timer</string> | ||
| <string name="sleep_timer_set">Sleep Timer stellen</string> | ||
| <string name="sleep_timer_cancel">Sleep Timer beenden</string> |
There was a problem hiding this comment.
TODO: Check VLC translation. AFAIK they use a different word than "Sleep Timer". We might want to use the same terminology (German and English).
They use "Ruhemodus". Not sure if that is a good translation. But "Sleep Timer" is English. We might want to let the translators figure this out :)
| /** | ||
| * @param remainingTime the remaining sleep timer time, set to 0 to pause the player and | ||
| * disable the sleep timer | ||
| */ | ||
| public void onSleepTimerUpdate(final long remainingTime) { | ||
| } |
There was a problem hiding this comment.
Please ensure that the time unit is mentioned.
| final long remainingMinutes = java.time.Instant.now().until(sleepTimerEnd, | ||
| ChronoUnit.MINUTES); | ||
| UIs.call(playerUi -> playerUi.onSleepTimerUpdate(remainingMinutes + 1)); |
There was a problem hiding this comment.
is remainingMinutes + 1 used because until floors the value? If so, add an explaining comment.
| if (remainingTime == 0) { | ||
| binding.sleepTimerTextView.post(new Runnable() { | ||
| public void run() { | ||
| player.pause(); |
There was a problem hiding this comment.
this method is just used for UI updates. The pause() call should happen inside the player.
|
Thank you for this PR! I am not sure this is the right time to start adding this feature. NewPlayer is about to be integrated on the refactor branch and that will mean throwing away most of the current player code. So it might be better if you postpone this to after NewPlayer is integrated in NewPipe. |
3 participants
What is it?
Description of the changes in your PR
Adds a sleep timer menu with some predefined values.
The sleep timer pauses the video as soon as the timer runs out.
Closes
Fix #2830
Before/After Screenshots/Screen Record
Relies on the following changes
refactorbranch, can also port todevif usefulAPK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Will add APK links as soon as a maintainer approves the workflows for this PR.
Due diligence