Issue #660 - NightMode

[AnotherDaniel]

changed theme setting to nightmode selection, update isDarkTheme(), changed settings default to NightMode=Auto

Note: this change set only include localization changes for DE and EN languages, as I do not speak any of the others...

[AnotherDaniel]

Ok, now Codacity and I are in disagreement re readability of that if statement - anyways, tomorrow...

[AnotherDaniel]

Allright, the entire isDarkMode thing is a bit off anyways, now that I'm thinking about it. Will finally fix tomorrow.

[David-Development]

@AnotherDaniel Great, thank you! :) I'll take a deeper look at it later today! One thing to know about most nextcloud android apps is, that the translations are provided by transifex. Therefore developers can only modify the values/string.xml file. All other files are not to me modified by the developer manually as they will be overwritten by transifex.

@AndyScherzinger do you know if it is a problem for transifex that the translations were modified? Or will they be overwritten anyways?

[AndyScherzinger]

@David-Development not a problem afaik, transifex will simply overwrite them
@AnotherDaniel @David-Development the master file (values/strings.xml) is the only one that needs the new strings, the others won't lead to issues but would be erased over night after the PR has been merged to the master branch.

[AnotherDaniel]

Pushed a hopefully final change to fix the isDarkMode implementation - before I do anything else I will have to figure out how to run Codacity on my own, before I actuall push stuff.
Feeling off today, so earliest tomorrow - but would be delighted if the overall change is something you guys can pick up, let me know what you think!

[David-Development]

@AnotherDaniel Awesome work! 👍

I did a little refactoring. There is only one mayor thing that I changed.. I added the support for the OLED theme (again). Along with that, I changed the wording a little too. Therefore there are 4 "Themes" available now:

0: Dark / Light (Default)
1: Light
2: Dark
3: OLED

What do you think? If you don't like it, feel free to change it back or give some feedback :)

Btw: I merged the master branch in here.. therefore you might need to update Android Studio to 3.2.
If you run into any compile issues, let me know.. google changed a lot and I had some issues getting the project back up and running with the news version today. Hopefully everything works now..

[AnotherDaniel]

David, thank you for picking this up!

Wording, no real opinions - would be helpful if Google had some official guidance on what this kind of feature should be called; I guess I'll look around a bit just in case there is something.

I'd like the OLED thing to be a separate boolean switch, a modifier on the dark theme - I will try to create a branch for that the next few days, if that is ok for you!

[David-Development]

@AnotherDaniel Sounds good! However some concerns regarding the OLED Theme..

I'm still a little hesitant with adding an additional switch for OLED (if the user selected the Light or Auto Theme, this flag wouldn't have any effect on the light theme). On the other hand, adding it as a separate flag makes it more customizable for the user. Therefore I'm looking forward to the solution you'll come up with :)

And yes, for sure! Feel free to create a new branch!

One question: I tested the day/night mode on my device and it just changed the color to "night" mode (at exactly 10pm). This is quite late as it gets dark around 7-8pm here (Germany). Is this okay or should we try to investigate this a little more as well?

[AnotherDaniel]

I'll give it a try then :)

Re the switching time: 10pm is expected behavior, yes - judging by the Twitter app, which also uses this Android system auto switch feature I think; also happens at 10pm (and back to day at 6am I believe).

So we'd be consistent with what android does, and if someone goes to be earlier they can always switch manually ;-)

[David-Development]

@AnotherDaniel Okay, I see! Thanks for the clarification!

[David-Development]

@AnotherDaniel Found some color issues (e.g. podcast fragment) and noticed, that there are a few color variables that we didn't need anymore. I cleaned everything up a little. Hope that's okay with you? :)

[AnotherDaniel]

@David-Development you're funny :) - I won't say how little of all those color definitions and dozens of places where some of them are used I actually understand ;)
(also was astonished when I came upon that css file - yet another source of color/style definitions?)
I only dared to remove one so far, so anything that you throw out now I won't have to puzzle over tomorrow night...

[David-Development]

@AnotherDaniel ahah.. well, the css file has a different purpose. The rss news that we receive in the news app contain a HTML body content.

See REST API (get-items) in the return section under body. (e.g. "body": "<p>At first I have to say...</p>")

The content is in the HTML format and it contains links, images, videos etc. Therefore the only logical way of displaying it, is by using a webview and rendering the content into a small html-skeleton. That's what's happening in the getHtmlPage method of the RssItemToHtmlTask class (https://github.com/nextcloud/news-android/blob/144a88014b45aa62e7ecc0f122532dc40c4b4f6a/News-Android-App/src/main/java/de/luhmer/owncloudnewsreader/async_tasks/RssItemToHtmlTask.java). And in order to style the content according to the theme used by the app, we had to use css rules to make the webview look consistent to the rest of the app. That's why there are css/style definitions as well as the normal color/theme definitions. Hope that clarifies it a little better.

I think we even might be able to remove a couple more color attributes from the themes.xml as we had to define them there to be able to override them according to the current theme. By using the day-night mode this becomes somewhat redundant as we have two color.xml files. But I need to look into it again since there might be some conflicts due to the OLED Theme. But I think we can do this sort of cleanup after you're done with the additional switch :)

If you have any further questions or if I can clarify some things, let me know!

[AnotherDaniel]

@David-Development Thanks for the explanation above; I've another thing now that's opaque to me :(
There is a french-resource string value somewhere that's escaped buggily (token name token_mismatch_message, there are two consecutive \'s in there near the beginning: "Le jeton d\\'authentification ").
That problem could be fixed in the build files (I assume) with the previous Android Studio version, but I've updated just now - and now the build always re-generates those values-fr.xml files with that bad string - and I've no clue where tf it is getting that input from?!?
There is no file in the repository, outside of stuff in the ./build folders, that contains the token name or string values that would need to be fixed.

Any pointers as to where the build system is getting the data used to generate this token in values-fr.xml? I'm a bit stumped...

[David-Development]

@AnotherDaniel Yes.. there was an invalid translation in the Single Sign On Repo (Version 0.1). You can edit the build.gradle file and set the version from 0.1 to master-SNAPSHOT. That should fix this problem.

This line:
https://github.com/nextcloud/news-android/blob/master/News-Android-App/build.gradle#L104

[AnotherDaniel]

Hi @David-Development, I've made some progress - unfortunately I'm not quite sure about how to actually work with git/github yet I think.
I have create a pull request in my own fork, for the delta between the last nightmode state and the changes for the OLED switch:
AnotherDaniel#1

Description:
Change set for adding a boolean settings switch for dark theme black background for OLED screens.
The background changing is applied to all non-dialog-like screens:

news reader list
settings screen
add new feed screen

It is not applied to any popup-kind of dialogs:

left sidebar
settings button menu
server settings screen
about/change dialog

It is also not (yet) applied to the built-in webbrowser view for articles.

I am quite pleased for the moment - will look at the webbrowser view next, undecided on whether to touch the pop-up dialog things or if I actually like them to stay a bit lighter...
Please let me know what you think, and how I actually should have done with this from a github workflow perspective :-)

[David-Development]

@AnotherDaniel Awesome! Great work! 👍

I don't think that we need to change the left sidebar or any popup dialogs as they're only visible for a short period of time and it would look weird if they were pitch black as well.. Therefore I think it's just fine the way it is right now! :)

I did some refactoring (make the oled switch apply the black color to the webview as well).

I also used the AppThemeOLED again as it makes it easier to change the background color without having to set the color manually in each view/fragment. Let me know what you think! I created another branch and I created a PR for your branch. Feel free to merge it if you're okay with the changes.

PR: https://github.com/AnotherDaniel/news-android/pull/2

[AnotherDaniel]

@David-Development Right, this a lot more elegant! Ok the next question would be: are you ok with getting that into the next release of the app? :)
This is all I wanted re the night mode request, but please let me know if there is anything else I should do to wrap this up.
Otherwise, I might just look at the issue list and maybe try my hand at some there...

[David-Development]

@AnotherDaniel Yes, I'll merge everything later.. I found some issues that the theme is not properly applied sometimes.. (e.g. when you use oled and dark theme and switch back to auto mode the list is still black). If you restart the app manually it's good..
Trying to figure out why this happens exactly.. looks like some caching issues.

Otherwise, I might just look at the issue list and maybe try my hand at some there...

Awesome! 👍Looking forward to answer some questions and to merge pull requests! :)

are you ok with getting that into the next release of the app? :)

Yes, for sure! As we did some major layout changes this will be the perfect time for your feature to be released as well! Are you okay with me putting you on the contributor list of this repo? (You'll be mentioned in the changelog as well)

[David-Development]

Looks like the line

int nightModeFlags = context.getResources().getConfiguration().uiMode & 
                                                             Configuration.UI_MODE_NIGHT_MASK;

is returning "dark theme" right at the beginning when using auto mode during day-time.. how annoying.. just a couple of milliseconds later, it's changed to "light theme" correctly.. Need to look into it further..

[AnotherDaniel]

@David-Development Hm sounds like something that might vary per device, timing wise - haven't seen this yet, I only had the switch to/from OLED not work once, but couldn't reproduce. I'll also look around a bit...

Sure you can mention me in the contributor list - although it would be more honest to credit stackoverflow...

[David-Development]

@AnotherDaniel Haha.. well, that's what every developer does, isn't it? :)

Yeah.. I kind of noticed it a couple of times... tried to add some workarounds for the few cases when it occurs... we'll just have to keep an eye on it.

[AnotherDaniel]

@David-Development One thing I seem to have forgotten with the oled-mode switch: I think the setting should be 'false' by default - so in the pref_display.xml file, line 17 we should put this:
android:defaultValue="false"

(don't think this is worth a pull request or even a branch commit...)

[David-Development]

I'm still having some annoying issues with the auto mode.. When I open the detail view (webview) for the first time (after 10pm) it's using the light theme.. When I go back and open the detail view again, it's using the dark theme.. I think this has something to do with caching on the Android site.. I'm wondering if we can make this more stable somehow.. I wasn't able to find a callback such as onThemeChanged or something like that..

[AnotherDaniel]

@David-Development Crap - will also search for solutions

[AnotherDaniel]

@David-Development Hm; I've just tried this with forceReloadCache set to true in line 95 of RssItemToHtmlTask.java
switch(ThemeChooser.getInstance(context).getSelectedTheme(context, true)) {

Got the black browse brackground on the first go. But: this was the first time I've tried this, didn't attempt with forceReloadCache:false - so no idea yet whether it simply works for me, or whether that flag makes a difference 🔨

[David-Development]

@AnotherDaniel This shouldn't make any difference as we only cache the selected theme (e.g. Auto / Light / Dark). Caching this is fine. When the auto-mode is enabled, it'll try to figure our whether the current theme is dark or light (depending on daytime) - and that's were the problem occurs.. As far as I can tell, the following line causes the problem as it returns the wrong value sometimes.. I think Google is doing some async work in the background to figure out which theme to choose (e.g. check location / fallback to time in our case). Our code is checking that flag before they figured out whether it's day or night.. I'm not 100% sure though.. need to investigate a little more.

int nightModeFlags = context.getResources().getConfiguration().uiMode & Configuration.UI_MODE_NIGHT_MASK;

[AnotherDaniel]

@David-Development Do you get that on the device emulator or on your actual mobile? Does it work if you don't go to the detail view immediately after 10pm, but wait for say 2 minutes?
(admit I didn't try on an actual device yet, have been using a Pixel 2 XL emulator so far)

[David-Development]

Can't reproduce it anymore..
I ran into this issue mostly on my phone (Android 7) but also on the emulator (also Pixel 2 XL - Android 9 Pie).

Anyways.. I'm writing some tests to automate this kind of testing a little more :)

[David-Development]

@AnotherDaniel I just made some changes to this branch.. (added tests for nightmode including some fixes for bugs that I found during testing / also brought it up to speed with the master branch). I tried to push my changes to your fork but I noticed, that the branch is not there anymore (in your fork). And I also don't have permission to push to your fork. If you give me permission to your fork I might be able to restore the branch by pushing my history to it.

As I used the app during the past couple of days the switch between day/night worked reliable. So I guess we can merge this soon :)

[AnotherDaniel]

@David-Development thank you very much for tidying up after me - very interested to see what you did!
Made you a collaborator on my fork (deleted it a couple of days ago and did a clean one, as I messed up my master)

[David-Development]

@AnotherDaniel Thank you! 👍 I'll close this one and create a new PR as GitHub won't pickup the new branch (changes) automatically (it's a known issue: isaacs/github#168 (comment)).