Send a warning notification if device is unplugged and at low battery #12330

Cublibre · 2021-04-07T20:29:02Z

Related to #12239

Description

Imported Xamarin.Essentials.Battery to osu.Game to detect battery information on all platforms. If the device's battery level is below some value (currently 20%) and not plugged in, sends a notification similar to the muted notification (only once per game session, upon loading a beatmap). Doesn't detect battery information on desktop versions because .NET standard is unsupported by Xamarin.Essentials.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Visual tests in TestScenePlayerLoader.cs:TestLowBatteryNotification with different battery configurations
Deployed to physical Pixel 2

Co-Authored-By: Marlina José marlina@umich.edu

- Uses Xamarin.Essentials in osu.Game.PlayerLoader to check battery level - Encapsulated battery checking in the public BatteryManager class so battery level and plugged in status can be accessed and edited in TestPlayerLoader - When checking battery level, catch NotImplementedException thrown by Xamarin.Essentials.Battery on non-mobile platforms - Added visual unit tests for battery notification To mock battery status and level, we had to define a batteryManager object in TestPlayerLoader and add a new function ResetPlayerWithBattery() Co-Authored-By: Marlina José <marlina@umich.edu>

frenzibyte · 2021-04-07T21:20:41Z

Importing a platform-specific package and using it in the main osu.Game project shouldn't be the way to add this.

One common way to do it would be to make an abstract BatteryStatus class and cache for PlayerLoader to access via DI, and implement in both osu.iOS and osu.Android, in the same manner as OsuGame.CreateUpdateManager() is abstracted and implemented in osu.Desktop.

For test case, you would make a dummy battery status class and cache from the test scene.

frenzibyte

As above.

Cublibre · 2021-04-08T03:22:21Z

Thanks for the review. I added an abstract PowerStatus class as you said, but I'm having trouble registering the PowerStatus dependency so PlayerLoader can access it. I read the article on Dependency Injection and tried using the Cached and Resolved attributes. Currently I defined a readonly PowerStatus field in OsuGameDesktop,OsuGameAndroid, and OsuGameiOS like so:

OsuGameDesktop.cs

internal class OsuGameDesktop : OsuGame
{
    // ...
    [Cached]
    protected readonly PowerStatus PowerStatus = new DesktopPowerStatus();
    // ...
    [Cached]
    private class DesktopPowerStatus : PowerStatus
    {
        public override double ChargeLevel => 1;
        public override bool IsCharging => true;
    }
 }

PlayerLoader.cs

namespace osu.Game.Screens.Play
{
    public class PlayerLoader : ScreenWithBeatmapBackground
    {
        [Resolved]
        private PowerStatus PowerStatus { get; set; }
        // ...
    }
}

How should I should be registering PowerStatus?

The commit: Cublibre@3289f9a

peppy · 2021-04-08T05:31:49Z

Honestly why don't we just keep this simple and make it a global notification rather than only on entering gameplay?

In addition, does your operating system not tell you this already?? iOS will show battery warnings at a system level, so I don't believe we even want this implemented for the iOS platform. Is it not a thing on android?

LittleEndu · 2021-04-08T05:38:11Z

It most certainly is a thing on any platform, although on android it might depend on the manufacturer. But isn't the idea here that the in game notification would appear before the OS one and be less intrusive. If the OS shows the notification it would unexpectedly pause gameplay.

peppy · 2021-04-08T05:39:13Z

Fair, but that should be documented if so, along with the cutoffs for the operating systems showing the notifications themselves.

peppy · 2021-04-08T05:43:18Z

@Cublibre the code you have looks quite correct. what is the issue you're running into with DI?

One thing I've noticed is that it will likely crash on non-mobile platforms because the cached PowerStatus is not available. You will want to use the [Resolved(canBeNull: true)] to get around this.

- Removed the Xamarin.Essentials package from osu.Game and added it to osu.iOS and osu.Android only. - iOS and Android implementations use Xamarin.Essentials.Battery, while the Desktop implementation only returns 100% battery for now. - Added a BatteryCutoff property to PowerStatus so it can be different for each platform (default 20%, 25% on iOS)

Cublibre · 2021-04-09T00:13:15Z

I added a BatteryCutoff property to the PowerStatus class so it can be different across platforms.
The original issue showed that the iOS popup appears at 20%. For Android it seems to be different depending on the manufacturer- Samsung seems to let you choose when the warning appears while the Pixel 2 doesn't.

sorry about the style fixes... I'm using JetBrains Rider from now on.

marlinabowring · 2021-04-09T15:28:22Z

Right now, we are trying to cache the PowerStatus dependency in OsuGameBase and initialize it within each platform (Android, iOS), but we are running into an issue as seen in the CI logs related to a Null value within an argument. We are initializing the PowerStatus within OsuTestBrowser, but is there somewhere else we should be doing this?

frenzibyte · 2021-04-09T15:36:12Z

This is the OsuTestSceneTestRunner which is used to run test units for CI causing an attempt to cache a null PowerStatus which is not allowed, I have a better suggestion for doing all of this in a pending review, wait a bit.

frenzibyte

Minor code quality notes. Haven't tested this yet.

osu.Game/OsuGameBase.cs

osu.Game/Screens/Play/PlayerLoader.cs

osu.Game/Utils/PowerStatus.cs

osu.Game/osu.Game.csproj

osu.Desktop/OsuGameDesktop.cs

osu.Game/Screens/Play/PlayerLoader.cs

osu.Game/Configuration/SessionStatics.cs

osu.Game.Tests/Visual/Gameplay/TestScenePlayerLoader.cs

…tatus class in test scene

frenzibyte · 2021-04-11T07:20:06Z

I've tested this on both Android and iOS, have made pretty small changes regarding code style and little adjustments which can be cherry-picked here: bb7c40b 4537a7e ad68a88 a040158 6dfa03b

Looks alright to me otherwise (the class might need to be renamed though) and behaves as been written. 👍🏼

Cublibre · 2021-04-11T15:50:37Z

Thanks. How do I cherry-pick those commits? They don't exist on any branch so git cherry-pick doesn't work.

frenzibyte · 2021-04-11T15:56:40Z

Oh whoops forgot about that, they're from this branch you can checkout here: https://github.com/ppy/osu/compare/master..frenzibyte:low-battery

Cublibre · 2021-04-11T18:07:47Z

I've pushed the changes.

frenzibyte · 2021-04-11T19:08:40Z

Missing this commit frenzibyte@bb7c40b?

Cublibre · 2021-04-11T19:22:27Z

My bad, I didn't realize cherry-pick doesn't include the first commit in a given range. Thanks for pointing that out!

frenzibyte

Looks good to me.

As an aside, if you wish to further contribute here, I would recommend reading the contributing guidelines if you haven't already, as I see you've PR'd this off the master branch, instead of a separate branch.

osu.Game/Utils/PowerStatus.cs

osu.iOS/osu.iOS.csproj

osu.Android/osu.Android.csproj

osu.Game/Utils/PowerStatus.cs

dismissing for above points

peppy · 2021-04-15T09:56:31Z

osu.Game/Configuration/SessionStatics.cs

@@ -16,6 +16,7 @@ protected override void InitialiseDefaults()
        {
            SetDefault(Static.LoginOverlayDisplayed, false);
            SetDefault(Static.MutedAudioNotificationShownOnce, false);
+            SetDefault(Static.LowBatteryNotificationShownOnce, false);


One thing to note is that if the app is never closed, this will not be reset, meaning potentially it will not be displaying when the user may want it to. I'm not sure if that's an issue which should be solved in this PR, or whether it's a SessionStatics issue (one could argue the other features using this class suffer from the same flaw, and should be reset when the app goes inactive or after a certain period of time).

Leaning towards the latter.

pull-request-size bot added the size/L label Apr 7, 2021

frenzibyte requested changes Apr 7, 2021

View reviewed changes

frenzibyte added area:gameplay platform/mobile labels Apr 7, 2021

Cublibre added 3 commits April 8, 2021 20:28

Fixed code style

493c095

trim whitespace

6b6a71d

Fixed indentation

59d13b0

sorry about the style fixes... I'm using JetBrains Rider from now on.

frenzibyte requested changes Apr 9, 2021

View reviewed changes

Remove setters, cache CreatePowerStatus() and use a dummy LocalPowerS…

08311ab

…tatus class in test scene

Cublibre requested a review from frenzibyte April 10, 2021 04:56

Merge branch 'master' into master

1fd4cb8

frenzibyte added 4 commits April 11, 2021 13:18

Cache power status at base instead

2b947a4

Adjust documentation

3d85dc1

Make power status properties abstract

07ee1b4

Add expected output in test case rather than determining internally

cb947a3

Reorder method declaration

419fd44

Remove Xamarin.Essentials package from main project

b7e16c2

frenzibyte previously approved these changes Apr 11, 2021

View reviewed changes

peppy requested changes Apr 12, 2021

View reviewed changes

Cublibre added 2 commits April 12, 2021 10:52

Refactor PowerStatus (now called BatteryInfo)

43b97fe

Remove IsLowBattery

f66306a

Cublibre requested a review from peppy April 12, 2021 16:08

peppy reviewed Apr 15, 2021

View reviewed changes

peppy approved these changes Apr 15, 2021

View reviewed changes

peppy merged commit 5b06a68 into ppy:master Apr 15, 2021

peppy mentioned this pull request Apr 15, 2021

SessionStatistics is not reset on inactivity #12424

Closed

HelloYeew mentioned this pull request Apr 15, 2021

Make a game pause when alert window appear on iOS (or mobile) #12239

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send a warning notification if device is unplugged and at low battery #12330

Send a warning notification if device is unplugged and at low battery #12330

Cublibre commented Apr 7, 2021 •

edited by frenzibyte

Loading

frenzibyte commented Apr 7, 2021 •

edited

Loading

frenzibyte left a comment

Cublibre commented Apr 8, 2021

peppy commented Apr 8, 2021

LittleEndu commented Apr 8, 2021

peppy commented Apr 8, 2021

peppy commented Apr 8, 2021

Cublibre commented Apr 9, 2021

marlinabowring commented Apr 9, 2021

frenzibyte commented Apr 9, 2021 •

edited

Loading

frenzibyte left a comment

frenzibyte commented Apr 11, 2021

Cublibre commented Apr 11, 2021

frenzibyte commented Apr 11, 2021 •

edited

Loading

Cublibre commented Apr 11, 2021

frenzibyte commented Apr 11, 2021

Cublibre commented Apr 11, 2021

frenzibyte left a comment

peppy Apr 15, 2021

Send a warning notification if device is unplugged and at low battery #12330

Send a warning notification if device is unplugged and at low battery #12330

Conversation

Cublibre commented Apr 7, 2021 • edited by frenzibyte Loading

Description

Type of change

How Has This Been Tested?

frenzibyte commented Apr 7, 2021 • edited Loading

frenzibyte left a comment

Choose a reason for hiding this comment

Cublibre commented Apr 8, 2021

peppy commented Apr 8, 2021

LittleEndu commented Apr 8, 2021

peppy commented Apr 8, 2021

peppy commented Apr 8, 2021

Cublibre commented Apr 9, 2021

marlinabowring commented Apr 9, 2021

frenzibyte commented Apr 9, 2021 • edited Loading

frenzibyte left a comment

Choose a reason for hiding this comment

frenzibyte commented Apr 11, 2021

Cublibre commented Apr 11, 2021

frenzibyte commented Apr 11, 2021 • edited Loading

Cublibre commented Apr 11, 2021

frenzibyte commented Apr 11, 2021

Cublibre commented Apr 11, 2021

frenzibyte left a comment

Choose a reason for hiding this comment

peppy Apr 15, 2021

Choose a reason for hiding this comment

Cublibre commented Apr 7, 2021 •

edited by frenzibyte

Loading

frenzibyte commented Apr 7, 2021 •

edited

Loading

frenzibyte commented Apr 9, 2021 •

edited

Loading

frenzibyte commented Apr 11, 2021 •

edited

Loading