eknoes / golem-android-reader

An unofficial android application for german tech-site golem.de
https://play.google.com/store/apps/details?id=de.eknoes.inofficialgolem
GNU General Public License v3.0
20 stars 6 forks source link

Confusing loading state in Battery Saver #50

Closed graciouselectric closed 4 months ago

graciouselectric commented 5 months ago

Dear developer,

I noticed that when I enable Battery Saver on Android 8.1, the indeterminate ProgressBars in the app are not properly shown. This is a known problem in Android API level <28, see e.g. this StackOverflow question. Battery Saver disables animations, also on progress bars on these versions. This is quite confusing because the loading state is not properly represented. It is fixed in later Android versions, where progress bars do appear and animate.

Looking at the code, indeterminate progress bars are created here:

className='de.eknoes.inofficialgolem.ArticleFragment', lineNumber=126

To fix this issue, one can check ValueAnimator.areAnimatorsEnabled() and provide a different UI element, such as a text label, when animations are disabled.

I also recorded a video, showing the issue in practice:

https://github.com/eknoes/golem-android-reader/assets/165037835/f11fe866-8789-4298-ba88-968f61d2c39a

eknoes commented 4 months ago

Unfortunately I can not reproduce that in the Emulator. Are you able to test 1536026 ?

eknoes commented 4 months ago

I uploaded a debug build with the fix included here: https://dl.eknoes.de/golem-fix-50.apk

graciouselectric commented 4 months ago

Hi @eknoes, thanks for your response! Sadly the new build doesn't fix it. I think I see the mistake: you're checking whether we're running a pre-O Android version, but this issue (and my testing) occurs on O. The check should be whether we're running pre- Android P.

eknoes commented 4 months ago

Ok. Thanks for the test.

The condition is there to the check if the Android version is newer than O, so also P should be included. Only if that is the case, the ValueAnimator function is called. So on P, (Build.VERSION.SDK_INT < Build.VERSION_CODES.O || ValueAnimator.areAnimatorsEnabled()) should rely on the second part of the condition, as the first part is false. Below Android O, the first part is true, so the second part is never evaluated.

I need the check, as the ValueAnimator is only available in O and newer, so if it is older than O it should not check the ValueAnimator but just show the progressBar.

Unfortunately, I currently do not really have the time to take a deeper look. I hope I can investigate it in the next month. If you have more hints, or want to try to make a PR feel free to do so :)

graciouselectric commented 4 months ago

Ah, I see, that clarifies the code.

Looking more closely at the code, I now realize I think the element in the video is not directly an instance of ProgressBar, rather, it's created by the lines following your fix:

if (!mArticleSwipeRefresh.isRefreshing()) {
     mArticleSwipeRefresh.setRefreshing(true);
}

I don't think I've seen the progress bar in the app then. Can you explain when it is normally visible?

eknoes commented 4 months ago

It should be visible when opening an article, slightly under the top bar. I added the same fix for the SwipeRefresh, would you be able to test it again? I updated the APK at https://dl.eknoes.de/golem-fix-50.apk to include the fix.

graciouselectric commented 4 months ago

The swipe to refresh UI now doesn't show in the article, it does in the article list though, as expected from the code.

https://github.com/eknoes/golem-android-reader/assets/165037835/c9da1236-e712-4fc3-aff7-df68c850d1ea

On another note, I'm a little confused I don't see the progress bar under the top bar, nor on other API levels with BS turned off.

eknoes commented 4 months ago

I am not using the app regularly, so I had to check again with the progress bar. It is not visible anymore since the swipe-to-refresh layout has been introduced 6 months ago. So I removed the code now :)

I pushed the changes, added the same condition to the animation in the ListView, and will publish the new version on F-Droid and the Play Stores Beta track. Thanks for reporting the issue and suggesting the fix!