ankidroid / Anki-Android

AnkiDroid: Anki flashcards on Android. Your secret trick to achieve superhuman information retention.
GNU General Public License v3.0
8.67k stars 2.24k forks source link

Night mode does not work at all in AnkiDroid 2.16alpha84. #12404

Closed david-allison closed 2 years ago

david-allison commented 2 years ago

Night mode do not work at all in AnkiDroid 2.16alpha84.

Originally posted by @pavreh in https://github.com/ankidroid/Anki-Android/issues/11982#issuecomment-1247607681

david-allison commented 2 years ago

Rebase in:

Regression Test: AnkiActivityTest.kt ```kotlin /* * Copyright (c) 2022 David Allison * * This program is free software; you can redistribute it and/or modify it under * the terms of the GNU General Public License as published by the Free Software * Foundation; either version 3 of the License, or (at your option) any later * version. * * This program is distributed in the hope that it will be useful, but WITHOUT ANY * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A * PARTICULAR PURPOSE. See the GNU General Public License for more details. * * You should have received a copy of the GNU General Public License along with * this program. If not, see . */ package com.ichi2.anki import android.content.Intent import android.content.res.Configuration import androidx.test.ext.junit.runners.AndroidJUnit4 import com.ichi2.themes.Theme import com.ichi2.themes.Themes import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.equalTo import org.junit.Test import org.junit.runner.RunWith @RunWith(AndroidJUnit4::class) class AnkiActivityTest : RobolectricTest() { @Test fun themeChangeIsValid() { val activity = startActivityNormallyOpenCollectionWithIntent( AnkiActivity::class.java, Intent() ) assertThat(Themes.currentTheme, equalTo(Theme.LIGHT)) val newConfig = Configuration(activity.resources.configuration) newConfig.uiMode = 33 activity.onConfigurationChanged(newConfig) assertThat(Themes.currentTheme, equalTo(Theme.BLACK)) } } ```
git bisect reset
git bisect start
git bisect bad
git bisect good ceefefb2305db79b1d34637be73373741b1701f9
git bisect run ./gradlew AnkiDroid:testPlayDebug --tests "*themeChangeIsValid*"
Bisect details ``` cdaa607bf1d4f4af8570412fd6a6f67e38d073cc is the first bad commit commit cdaa607bf1d4f4af8570412fd6a6f67e38d073cc Author: Mike Hardy [redacted email] Date: Tue Sep 13 14:58:14 2022 -0500 Dependency updates 20220913 (#12393) * build(deps): bump jackson-databind from 2.13.3 to 2.13.4 Bumps [jackson-databind](https://github.com/FasterXML/jackson) from 2.13.3 to 2.13.4. - [Release notes](https://github.com/FasterXML/jackson/releases) - [Commits](https://github.com/FasterXML/jackson/commits) --- updated-dependencies: - dependency-name: com.fasterxml.jackson.core:jackson-databind dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] * build(deps): bump AppIntro from 6.1.0 to 6.2.0 Bumps [AppIntro](https://github.com/AppIntro/AppIntro) from 6.1.0 to 6.2.0. - [Release notes](https://github.com/AppIntro/AppIntro/releases) - [Changelog](https://github.com/AppIntro/AppIntro/blob/main/CHANGELOG.md) - [Commits](https://github.com/AppIntro/AppIntro/compare/6.1.0...6.2.0) --- updated-dependencies: - dependency-name: com.github.AppIntro:AppIntro dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] * build(deps): bump appcompat from 1.4.2 to 1.5.1 Bumps appcompat from 1.4.2 to 1.5.1. --- updated-dependencies: - dependency-name: androidx.appcompat:appcompat dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] * build(deps): bump mockito-inline from 4.7.0 to 4.8.0 Bumps [mockito-inline](https://github.com/mockito/mockito) from 4.7.0 to 4.8.0. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](https://github.com/mockito/mockito/compare/v4.7.0...v4.8.0) --- updated-dependencies: - dependency-name: org.mockito:mockito-inline dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] * build(deps): bump Android-Image-Cropper from 4.3.1 to 4.3.2 Bumps [Android-Image-Cropper](https://github.com/CanHub/Android-Image-Cropper) from 4.3.1 to 4.3.2. - [Release notes](https://github.com/CanHub/Android-Image-Cropper/releases) - [Changelog](https://github.com/CanHub/Android-Image-Cropper/blob/main/CHANGELOG.md) - [Commits](https://github.com/CanHub/Android-Image-Cropper/compare/4.3.1...4.3.2) --- updated-dependencies: - dependency-name: com.github.CanHub:Android-Image-Cropper dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] * build(deps): bump core-ktx from 1.8.0 to 1.9.0 Bumps core-ktx from 1.8.0 to 1.9.0. --- updated-dependencies: - dependency-name: androidx.core:core-ktx dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> AnkiDroid/build.gradle | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) bisect found first bad commit% ```

@mikehardy bad commit is 5473fb471f128044f7d025c3219206b4ebe11d75 .

androidx.appcompat:appcompat:1.4.2 -> 1.5.1 Added in https://github.com/ankidroid/Anki-Android/pull/12343

Want to fix-forward, or revert?

mikehardy commented 2 years ago

fix-forward, we're still in alpha. For beta or or stable would just revert. I always worry when appcompat moves a minor version, I was half expecting this...

From the PR:

Standard dependency updates, but moving core-ktx and appcompat may be "interesting". Hopefully not

So, it is "interesting". Joy :-)

https://developer.android.com/jetpack/androidx/releases/appcompat#1.5.0 says:

This stable version includes improvements to night mode stability

Possible hit: https://issuetracker.google.com/u/1/issues/239882532 (noting @pavreh lists device as Android 12 typically, but @david-allison your android version is unknown. Is it 12?). This hypothesis may be checked via test on Android 11 vs Android 12.

david-allison commented 2 years ago

I've posted a failing unit test above. I was testing on an Android 11 physical device

mikehardy commented 2 years ago

Got it re: Android 11. Glad we're not dealing with the above issue I linked then - or (alternative read) perhaps it will be a multi-bug with different effects.

If that's the case then, my reading of the changelog from appcompat 1.4.x to 1.5.x was that something about the attaching of contexts, their resources, their precedence, and how changes propagated from changes in the same, is different between 1.4.x and 1.5.x. So if I were triaging I would look very carefully at the context objects that are in play as uiMode changes

david-allison commented 2 years ago

I had a brief look, there was a single recursive call on onConfigurationChanged(newConfig) which could be problematic (recursive both in 1.4.2 and 1.5.1).

making an immutable copy of the config (Configuration(newConfig)) before calling to super appeared to fix this: but I'd like an investigation into what went wrong.

I'm not going to have time to take this further.

BrayanDSO commented 2 years ago

Oh boy. I'll look this on the weekend if I get the time

JalinWang commented 2 years ago

On Android11, with alpha84 It works if the system is in dark mode when starting. However, it doesn't response to the change of uiMode. Killing then restarting is ok.

JalinWang commented 2 years ago

As a side-effect, we now get configuration updates that are (correctly) missing our overrides. To counteract this, we let the delegate re-apply overrides and then we replace the configuration update contents with the desired overridden configuration contents.

https://android-review.googlesource.com/c/platform/frameworks/support/+/2137592/6#message-51139a0652cac062e09bf0185413dc938b30024a

It seems to be the cause.

BrayanDSO commented 2 years ago

I'm not gonna have time to fix this anytime soon. If someone else would like to take this, it would be very appreciated

JalinWang commented 2 years ago

This bug is introduced by the delegate who applies the overwritten config to the activity. It's fixed by https://android.googlesource.com/platform/frameworks/support/+/335eddbfe69984a079881274ebb72402f9adaf72%5E%21/#F0 which included in appcompat:1.6.0-rc01. I'll make PR when 1.6.0 is stable.

mikehardy commented 2 years ago

Hey @JalinWang fantastic! I don't see any problem with including an rc dep as a PR now if it really fixes things. Historically the rc -> stable transition for appcompat appears to be about 3 weeks, and dependabot will let us know to bump it to stable automatically so there won't be any follow-on, and this will be fixed now