Automattic / simplenote-android

Simplenote for Android
https://simplenote.com
GNU General Public License v2.0
1.78k stars 301 forks source link

Dark theme: Dark at night only doesn't change note list theme when blank editor screen is open #836

Open rachelmcr opened 5 years ago

rachelmcr commented 5 years ago

Expected

When the Dark at night only theme is selected, I expect the theme to change throughout the app at 6am and 10pm regardless of what screen is open.

Observed

If I have the note editor open to a new, blank note when the app switches between light/dark themes, the note editor changes theme but the note list does not (unless the app is closed and reopened).

Reproduced

  1. In Settings > Theme in the app, set the theme to Dark at night only.
  2. In your device settings, set the time to 5:59am.
  3. Go back to the app and confirm that it shows a dark theme.
  4. Create a new note in the app but don't add any content.
  5. When the time hits 6:00am, confirm the note editor switches to the light theme.
  6. Close the editor to go back to the note list. Note that it still shows the dark theme even though it should have switched to light theme.
  7. Close the app and reopen it, and confirm that the light theme is showing throughout the app.
Make Model Android Version App Version
Motorola e5 play 8.1.0 2.1-rc-1
theck13 commented 5 years ago

I don't think I'm able to reproduce that behavior. I believe the expected behavior is shown in the animation below. Can you confirm, @rachelmcr?

rachelmcr commented 5 years ago

That looks like the expected behavior, but it's different from what I'm seeing on my device. Here's a quick GIF:

20191010_060110

As you can see, the editor switches to light theme (and remains in the light theme when I reopen it) but the note list keeps the dark theme.

However, this is a fairly edge case scenario — in other tests where I added content to the new note or performed other actions the whole app switched themes as expected — so this could be a very low priority or wontfix issue unless others are running into it.

theck13 commented 5 years ago

According to the documentation, the Light or Dark theme will be applied to all started activities (i.e. the note list screen) though it appears to not be doing that consistently for all devices. Since it is such an edge case scenario, let's put it in the Simplenote Groundskeeping project and address it during the next rotation unless it becomes a common problem.

It should be noted that the MODE_NIGHT_AUTO constant used for the Dark at night only setting has been deprecated meaning that feature may need to be removed from Simplenote Android altogether.

ParaskP7 commented 3 years ago

👋 @rachelmcr !

I have picked-up this issue and currently investigating it. Can you please confirm that you still have this issue?

I saw @theck13 updating the AppCompat libraries on the 11th of October 19', just after your discussion, which ended on the 10th of October 19'. This library update, to 1.1.0 might have automatically fixed the issue for us, so I am trying to verify that by asking you to try again.

For more info, please check this Changing themes in-app link and the below note within:

Note: Starting with AppCompat v1.1.0, setDefaultNightMode() automatically recreates any started activities.

I understand that the MODE_NIGHT_AUTO is different to the above mentioned default night mode, but IMHO it is worth the try.

On my side, I tried as well, but can't recreate the issue. Probably, if I had your Motorola device I would have been more lucky.


Also, as @theck13 said, the MODE_NIGHT_AUTO constant used for the Dark at night only setting has been deprecated for a long time now and may need to be removed on our side. The recommended approach is to use instead a more explicit setting, the MODE_NIGHT_AUTO_BATTERY. From the docs:

Night mode which uses a dark mode when the system's 'Battery Saver' feature is enabled, otherwise it uses a 'light mode'. This mode can help the device to decrease power usage, depending on the display technology in the device.

Question (❓ ): Do we have any number of how many users are using this setting? Maybe this will help us lean towards a decision.

ParaskP7 commented 3 years ago

Additionally, I created a branch, which updates the AppCompat library to its latest stable version, the 1.2.0.

The branch is found here: issue/836-dark-theme-notes-list

I did that because the newest version brings some updates to the night mode handling and because I wanted to play around a bit more with it, trying to reproduce this issue. After applying the update I didn't notice anything wrong and as such I would suggest keeping it. It might by itself, along with the 1.1.0 version, solve our night mode issue on some devices. But, otherwise, at least we get to be on the latest stable AppCompat version available.

👋 @theck13 , your thoughts?

theck13 commented 3 years ago

Since MODE_NIGHT_AUTO and MODE_NIGHT_AUTO_BATTERY do not have the same behavior, I think we should keep MODE_NIGHT_AUTO until it is no longer working even though it is currently deprecated.

Do we have any number of how many users are using this setting?

No.

👋 @theck13 , your thoughts?

Updating androidx.appcompat:appcompat from 1.1.0 to 1.2.0 seems fine as long as it doesn't change the current behavior. It looks like the namespace for tinting images needs to be changed after updating to that version. Since tinting is used throughout the app, we'll have to ensure changing the namespace does not affect all images which are tinted in both Light and Dark theme for Simplenote's supported Android versions.

ParaskP7 commented 3 years ago

Updating androidx.appcompat:appcompat from 1.1.0 to 1.2.0 seems fine as long as it doesn't change the current behavior. It looks like the namespace for tinting images needs to be changed after updating to that version. Since tinting is used throughout the app, we'll have to ensure changing the namespace does not affect all images which are tinted in both Light and Dark theme for Simplenote's supported Android versions.

1) Yeap, to the extend that I tested the app I didn't see any changes to the current behavior, but you can never be careful enough and I am sure I haven't tested extensively. 2) The tint namespace changed indeed, but this is mainly done by Android, and for compatibility purposes, I don't expect this will affect any of the images that are tinted. As far as I tested, I didn't observe any issues. But, again, I didn't test everything since this branch was just a proof of concept and maybe through it @rachelmcr can get an .apk to test this issue with her device again, before we put more effort into it.