Screen does not stay on #781

[Sp4rky001]

Addressing #781:

This issue asked for screen to stay on while music is playing but for my own needs, I wanted screen to stay on only when lyrics are showing. I created this solution to address both cases. The following changes were made to the redesign branch:

There is one concern I haven't addressed is that I did not take the lock screen into account. While on the lock screen, KeepScreenOn functionality remains the same.

/// KeepScreenOn
/// 
/// Added dependencies:
///   - keep_screen_on: ^3.0.0
///   - battery_plus: ^6.0.2
/// This functionality adds two options in settings under "Interactions":
///   - Keep Screen On: Main Setting
///   - Keep Screen On: Only while plugged in
/// 
/// Ties into the following actions:
///   - MusicPlayerBackgroundTask.play() -- Sets _isPlaying to true.
///   - MusicPlayerBackgroundTask.pause() -- Sets _isPlaying to false.
///   - LyricsScreen.draw() -- Sets _isLyricsShowing to true.
///   - LyricsScreen.dispose() -- Sets _isLyricsShowing to false.
///   - FinAmp constructor -- Creates an event listener for battery status change.  Sets _isPluggedIn value.
///   - KeepScreenOnOption -- Changes to this option re-evaluates the keepScreenOn status.
///   - KeepScreenOnWhilePluggedIn -- Changes to this option re-evaluates the keepsScreenOn status.

[Chaphasilor]

Hey, thanks for the PR, but have you seen #802? Could you please give a quick rundown about the differences between your solution and the other one?

Seems like you also consider battery life, and have a more global setting?

[Sp4rky001]

Hey, thanks for the PR, but have you seen #802? Could you please give a quick rundown about the differences between your solution and the other one?

Seems like you also consider battery life, and have a more global setting?

I'm sorry, I did not see #802, I kinda just worked on it on my own.

I think some of my differences are:

1. My solution seems to have more options. The user can choose to keep the screen on while the music is playing (addressing Screen does not stay on #781) or only when the lyrics are being displayed (my own preference).
2. I've also taken into account that even if lyrics are displayed, the screen will turn off if music is paused.
3. The KeepScreenOnWhilePluggedIn setting not only allows users to choose whether to keep screen on while running on battery power, but also indicates to them that KeepScreenOn takes the plugged in status into account without having to explain it in the description.
4. As much as possible, I consolidated much of the KeepScreenOn logic to the KeepScreenOnHelper component so that it is easily extensible to take more conditions into account. For instance, an additional check for whether the device is on the lock screen.
5. Outside of the files necessary to create the settings options, modifications to existing classes are mostly one-liners. For example, MediaPlayerBackgroundTask makes a 1-line call when play() is invoked and another when pause() is invoked.

[Chaphasilor]

Okay, that sounds good! I'll try to take a proper look at this later today, and if everything works, we can get this merged :)

[Sp4rky001]

Please give me any pointers that you might have. Perhaps I should be using events instead? For instance listen for a play and pause event. I just didn't know what events are available. I'm new to flutter. Are there navigation events that I can tap into?

[Sp4rky001]

I did some research about Navigator events and found that most of the articles point toward using NavigatorObserver and making the pages RouteAware.

Do you think it is worth it to do this overhaul in the app? By making the pages RouteAware, future developments that need to know the current active page can just hook into a navigation event.

Before doing this, I did also look into just adding another handler to the navigatorObservers: array in the Finamp class but I think that doing the RouteAware overhaul might be better for the long run?

[Chaphasilor]

So Finamp's navigation setup is outdated, yes. Back when James created it, the newer navigation stuff didn't exist yet.
Wasn't there a drawback to the new solution? Something related to deep linking? If not, we could attempt to refactor things :)

Regarding events, yes, there are events you can hook into. Take a look how the queue service and playback history service are doing it.

Also, please use a Logger object instead of debugPrint(). Take a look at the other services, it's just one line you need to add :)

I sadly didn't have time to try out the code today. Hopefully tomorrow!

[Sp4rky001]

So Finamp's navigation setup is outdated, yes. Back when James created it, the newer navigation stuff didn't exist yet. Wasn't there a drawback to the new solution? Something related to deep linking? If not, we could attempt to refactor things :)

Regarding events, yes, there are events you can hook into. Take a look how the queue service and playback history service are doing it.

Also, please use a Logger object instead of debugPrint(). Take a look at the other services, it's just one line you need to add :)

I sadly didn't have time to try out the code today. Hopefully tomorrow!

Thanks for the reply. I will change to logger instead.

Looking further into RouteAware, I don't think it is what I needed, unfortunately. My hope was that it would allow me subscribe to a navigation push/pop of a specific page but all examples use the current context of the page. So basically, Page 1 implements RouteAware and subscribes the Push/Pop of itself by passing its own context.

I will look into queue service and playback service to see how they are handling events. Perhaps that's the correct way to do this. Thanks for the suggestions!

[Sp4rky001]

Hmm... I'm almost there. I got my solution pretty clean. Aside from the stuff necessary to draw the settings UI, there are just two lines of code made to main() to make it work.

For the navigation event, I wrote a NavigationObserver in my helper class and it works, but only when split screen view is disabled.

When split view is on, Navigator doesn't fire an even when the lyrics screen is shown. I've looked through the scaffolding but haven't quite found where I might be able to hook into a navigation event.

[Chaphasilor]

I just took a look at the PR :)

I think you can revert to making the main Finamp object a const again (main.dart:121) since you've moved the initialization elsewhere now.

As for the navigation observer, you should probably just take a closer look at SplitScreenNavigatorObserver, since that handles the split screen logic. It should have the needed information. I'm not really familiar with the split screen implementation myself yet.

I'd prefer if you'd truncate the comment in keep_screen_on_helper.dart to a minimum, since most of the information provided there (like the dependency and version information, or features) will be outdated at some point, and the comment will probably be left forgotten. You could move the comments about which events are listened to above the relevant lines instead.

Oh and you've modified the Finamp models (the Hive/DB stuff), but haven't re-generated the actual generated code that is used by Hive. You can find the command for that in the contribution guidelines in the root of the repo.

Regarding functionality:
Everything seems to be working fine so far! The only thing I'm not happy with is the settings themselves, The biggest issue is the settings titles. "Main setting", and the use of colons feels wrong to me. I think you can just omit both, and use the descriptions to clarify things, if necessary. The settings page should probably also be renamed to "Behavior" instead of "Interactions", and then we can move the fast scroller setting to the layout settings page.
We can also make the default be keep on while on lyrics screen and if plugged in, that shouldn't hurt and might make sense.

I've also noticed you're using a different package for keeping the screen on. Could you try using the wakelock_plus package instead? That supports all platforms (not just Android and iOS), which would be nice to have! It also seems to be more actively maintained.

Let me know if you have any further questions!

[Sp4rky001]

I just took a look at the PR :)

I think you can revert to making the main Finamp object a const again (main.dart:121) since you've moved the initialization elsewhere now.

As for the navigation observer, you should probably just take a closer look at SplitScreenNavigatorObserver, since that handles the split screen logic. It should have the needed information. I'm not really familiar with the split screen implementation myself yet.

I did start to look at SplitScreenNavigatorObserver, although the only thing that might apply there is the queuePop() method which is explicitly called when creating the split screen. Otherwise, it seems to be the same as my KeepScreenOnNavigatorObserver class. I'll keep looking. Maybe the addPostFrameCallback() method from queuePop() is the solution?

I'd prefer if you'd truncate the comment in keep_screen_on_helper.dart to a minimum, since most of the information provided there (like the dependency and version information, or features) will be outdated at some point, and the comment will probably be left forgotten. You could move the comments about which events are listened to above the relevant lines instead.

Ok, I'll slim down the comments

Oh and you've modified the Finamp models (the Hive/DB stuff), but haven't re-generated the actual generated code that is used by Hive. You can find the command for that in the contribution guidelines in the root of the repo.

I didn't check in the generated code (model.g.dart) thinking it might be one of the commands you run when you do a build. I'll check it in.

Regarding functionality: Everything seems to be working fine so far! The only thing I'm not happy with is the settings themselves, The biggest issue is the settings titles. "Main setting", and the use of colons feels wrong to me. I think you can just omit both, and use the descriptions to clarify things, if necessary. The settings page should probably also be renamed to "Behavior" instead of "Interactions", and then we can move the fast scroller setting to the layout settings page. We can also make the default be keep on while on lyrics screen and if plugged in, that shouldn't hurt and might make sense.

Sounds good! I'll do all of this.

I've also noticed you're using a different package for keeping the screen on. Could you try using the wakelock_plus package instead? That supports all platforms (not just Android and iOS), which would be nice to have! It also seems to be more actively maintained.

Let me know if you have any further questions!

Ha ha, since I wasn't familiar with Flutter, I didn't go with wakelock_plus because it looked more complicated. I'll have a look at that other pull request and see how they did it there. I agree that dependencies that support more platforms is definitely better!

[Chaphasilor]

I think this doesn't actually change the default, just in which order the options appear. You can keep the order how it was, starting with disabled.
To change the default, change it here: https://github.com/jmshrv/finamp/pull/858/files#diff-00fd4ef72cab57c7db5c49ff8cd257918492ca5650d72bec14b3ee8560fcad54R114

[Sp4rky001]

yea, I looked up how to set default and I guess it's something like calling a constructor with the value?

[Chaphasilor]

There's no way to set the default of an enum. But we use the enum as a value for a setting, and there's a place in finamp_models.dart where we specify the default settings value to be used, if no value has been selected yet.
This will not change the selected value after the first start, unless you clear the app data and thereby delete the database.

[Sp4rky001]

Well, I spent hours trying to find a nice event that I can listen to for when lyricsScreen is shown in split screen mode, but I think it might be because split screen builds that side of the screen with navigatorkey instead of context, the events just don't fire as expected.

I even tried implementing RouteAware on the LyricsScreen but that didn't fire the didPop event (think I would've had to implement RouteAware on the PlayerScreen as well to make them report their push/pop to each other?) and it was more intrusive than just adding two calls in LyricsScreen.

So the only thing I haven't done so far is your suggestion to rename Interaction Settings to Behavior. That seems like a bit of refactoring.

I do have one more thing that I worked on which is that I noticed that many of the helpers are instantiated and registered as a singleton with GetIt. I created a branch where I did this with my KeepScreeOnHelper. I think it might be better than running my code as a static class? Let me know what you think. Here is my branch:

• main.dart
• keep_screen_on_helper.dart

[Chaphasilor]

Refactoring to a singleton seems fine :)

It's a pity the splitscreen stuff causes so much trouble, maybe we have to redo that at some point. It's also causing issues on Android TV (not officially supported yet).

Let me know once you're happy with the code, so that I can take a final look and merge it :)

[Sp4rky001]

I merged the singleton code into my branch and did a bit of cleanup. I think I'm satisfied with the state of this code. If you agree, then let's merge! Thank you!

[Komodo5197]

Yeah, the split screen stuff is kind of strange, but basically, the player side has it's own independent navigator stack, separate from the main Navigator we get from the MaterialApp. The navigator Key stuff isn't about building the player side, it's about redirecting route changes in the secondary player side navigator into the main navigator. This is why doing things like clicking the album name causes a new screen to show up in the left section instead of over the player. It's a very hacky method, but it seems to work well enough. The lyrics screen's slide transition code seems to directly add the full route instead of just pushing the name, so it bypasses the onGenerateRoute function and stays in the secondary navigator. I don't believe this is intentional, it just happened to work out. The SplitScreenNavigatorObserver's purpose is to handle the case where the main navigator has a player screen in its history stack below the top, and then you activate splitscreen. I couldn't find a way to remove this screen immediately, so the observer just pops it immediately whenever you navigate back to it. Anyway, this is all kind of moot if just watching the LyricsScreen state is doing fine, but I'd expect that just adding your NavigatorObserver to the scaffold Navigator, as well as the MaterialApp, would have made it work?

Regarding Android TV, what sort of issues are popping up? It's possible I just forgot to add a controller or something that doesn't have a default on Android TV. Something like that was happening on desktop, where we had code relying on a default global ScrollController that only exists on mobile. Or is there some sort of issue with having multiple Navigators?

[Chaphasilor]

@Komodo5197 thanks for the info and clarifications! I debated pinging you, but didn't want to bother you ^^

On Android TV it's currently not possible to move the focus/cursor out of the player panel. You start on the main panel and can navigate into the player panel, but not ou, of it. I think I tried it back with 0.9.7 or 0.9.8.
There's also a problem with the cursor becoming trapped on the progress slider, but that's probably unrelated.

[Sp4rky001]

Yeah, the split screen stuff is kind of strange, but basically, the player side has it's own independent navigator stack, separate from the main Navigator we get from the MaterialApp. The navigator Key stuff isn't about building the player side, it's about redirecting route changes in the secondary player side navigator into the main navigator. This is why doing things like clicking the album name causes a new screen to show up in the left section instead of over the player. It's a very hacky method, but it seems to work well enough. The lyrics screen's slide transition code seems to directly add the full route instead of just pushing the name, so it bypasses the onGenerateRoute function and stays in the secondary navigator. I don't believe this is intentional, it just happened to work out. The SplitScreenNavigatorObserver's purpose is to handle the case where the main navigator has a player screen in its history stack below the top, and then you activate splitscreen. I couldn't find a way to remove this screen immediately, so the observer just pops it immediately whenever you navigate back to it. Anyway, this is all kind of moot if just watching the LyricsScreen state is doing fine, but I'd expect that just adding your NavigatorObserver to the scaffold Navigator, as well as the MaterialApp, would have made it work?

Regarding Android TV, what sort of issues are popping up? It's possible I just forgot to add a controller or something that doesn't have a default on Android TV. Something like that was happening on desktop, where we had code relying on a default global ScrollController that only exists on mobile. Or is there some sort of issue with having multiple Navigators?

Scaffold Navigator... that might be it. you know, I suspected that it might be a different context/navigator stack that was causing my problem. I tried to look for where this other stack might be but couldn't find it. If I can find the Navigator in the scaffold code, I'll try there. I guess another problem causing me to miss it was that I didn't know where I could add the observer. All examples I've seen have been to add it to MaterialApp.

[Sp4rky001]

Thanks to @Komodo5197, I found the Navigator in player_split_screen_scaffold.dart and it indeed does have allow an observers option. I added my NavigatorObserver to it and it worked!

I'm quite happy with my code now as it's as unintrusive as possible. I'm uncertain if you think that the code might be too chatty. I output a line every time a change requires KeepScreenOn to reevaluate. I set the level at FINE, but if you want it FINER or even just remove, I'll make the change.

Thanks!

[Chaphasilor]

Chatty logging is fine (pun intended). At the moment we don't discern between the different levels anyway, so it wouldn't make a difference. Eventually we might want to limit the verbosity, but so far keeping all logs hasn't caused any issues. The important bit is using a proper logger at all, so that it's obvious what's producing the logs and everything is handled properly. And your logging is definitely sensible and helpful for debugging!

I'll merge the PR now since everything seems to be working fine, but if you wouldn't mind throwing in a fourth "keep screen on at all times" option, you could open another PR for that, just for completeness' sake ^^
Thanks for adding this feature! <3

[Sp4rky001]

Chatty logging is fine (pun intended). At the moment we don't discern between the different levels anyway, so it wouldn't make a difference. Eventually we might want to limit the verbosity, but so far keeping all logs hasn't caused any issues. The important bit is using a proper logger at all, so that it's obvious what's producing the logs and everything is handled properly. And your logging is definitely sensible and helpful for debugging!

I'll merge the PR now since everything seems to be working fine, but if you wouldn't mind throwing in a fourth "keep screen on at all times" option, you could open another PR for that, just for completeness' sake ^^ Thanks for adding this feature! <3

I'm adding the always on feature, but I think it if it's "Always On", I would need to check if the finamp app is in the foreground, right? I'm thinking what you mean is Always on, even when paused? So I think it might be "Always on while player is shown" or "While Player Is Shown"

Also, in terms of the order of the options, I went from least restrictive to most restrictive... except for disabled at the top, of course. So it would be "While Player is Shown", "While Playing", then "While Showing Lyrics"

[Chaphasilor]

I just meant always on, no matter which screen. But player screen as a condition also makes sense. Hmm.
I think if you request a screen lock, it will only apply as long as the app is in the foreground. But it might have to be re-applied after returning to the app.

The download service already has a lifecycle listener, if that is needed :)

[Sp4rky001]

I just meant always on, no matter which screen. But player screen as a condition also makes sense. Hmm. I think if you request a screen lock, it will only apply as long as the app is in the foreground. But it might have to be re-applied after returning to the app.

The download service already has a lifecycle listener, if that is needed :)

Ok, I'll write it as Always On and see what the behavior is.

[Sp4rky001]

I implemented the Always On feature. On testing, while the WakelockPlus is set to enabled, I navigated to the home screen of my phone. After 15 seconds (my settings), my screen dimmed and eventually went to sleep. Without changing anything, I brought the app back to the front and left it alone. After a full minute, screen stayed on.

So it seems that WakelockPlus may already take app lifecycle into account?

I committed my change to the same branch. Do you want me to create a new pull request from this branch or create a new one then pull request?

[Chaphasilor]

Great! Yeah either the plugin handles it or the system keeps track of requested wakelocks. Would be nice to know how iOS behaves. I might be able to try it on Windows tomorrow.

Ideally you'd rebase the additional commita onto redesign, but maybe Git is smart enough to figure it out. Feel free to open a PR and try it out ^^