Dark mode #322

TheLastProject · 2019-12-02T15:30:33Z

Fixes #298. Defaults to whatever the system theme is. When changing the value in settings, instantly swaps theme.

Yes, this is really all that needed to be done. Android did a pretty good job at making dark themes possible easily.

This change is

brarcher · 2019-12-04T07:02:19Z

Experimenting with this on an emulator, I find that if the dark theme setting is configured, then the app is launched clean, it works as expected. However, if the app was running already when the dark mode setting is changed, the app may not take the theme or may do so improperly. Here is an example.

System in light mode, app in light mode
Launch app, enter app settings
Configure app to use dark mode
Click back to go to main app activity

Note that the title text is really dark. A similar situation happens when switching from dark to light.

I'm not sure yet what is causing this behavior on only what appears to be the card title text. (The note text is correct). Are you able to duplicate this on your test device or emulator?

TheLastProject · 2019-12-04T07:04:13Z

I did run into this issue earlier but I thought I fixed it. Seems I did miss a case. I'll take another look at it, thank you

brarcher

Reviewed 8 of 11 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @TheLastProject)

a discussion (no related file):
I've only a few comments on the code. Let me know if you track down the theme change issue I mentioned, and I'll take a look again.

app/src/main/java/protect/card_locker/preferences/SettingsActivity.java, line 58 at r2 (raw file):

            findPreference(getResources().getString(R.string.settings_key_theme)).setOnPreferenceChangeListener(new Preference.OnPreferenceChangeListener() {
                @Override
                public boolean onPreferenceChange(Preference preference, Object o) {

I do not mind if the { is at the end of the line or on the next line. Kindly make it consistent in the file, though.

app/src/main/java/protect/card_locker/preferences/SettingsActivity.java, line 65 at r2 (raw file):

                        return true;
                    }
                    else if(o.toString().equals(getResources().getString(R.string.settings_key_dark_theme)))

Could the if/else if/ pattern be changed either to if/else if/else or if/if/? That is,

if ( /*check light theme*/) {
   // something
} else if (/*check dark theme*/) {
  //something
} else {
  //something
}
return true

or

if (/*check*) {
  //something 
  return true
}

if (/*check*/) {
  //something 
  return true
}

// last option
return true;

app/src/main/java/protect/card_locker/preferences/SettingsActivity.java, line 73 at r2 (raw file):

                    AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM);
                    getActivity().recreate();

If all paths end up recreating and returning true, why not do the common work at the end and return once?

TheLastProject · 2019-12-05T15:56:23Z

I'll deal with the code comments soon, thanks for reviewing.

I've only a few comments on the code. Let me know if you track down the theme change issue I mentioned, and I'll take a look again.

The issue became less prevalent with recent commits, but it still does occur on some elements (like the border around the about webview). I'm not sure why that happens so it may take some time until I can figure that out, if ever. I will notify you if I believe I found it though.

TheLastProject · 2019-12-05T19:24:10Z

Because a picture is worth more than a thousand words, this is the issue I mean: the dialog uses a dark theme after switching to light theme. I am still going to try to fix this but it... feels like a pretty weird issue.

TheLastProject

Reviewable status: 10 of 13 files reviewed, 4 unresolved discussions (waiting on @brarcher)

a discussion (no related file):

Previously, brarcher (Branden Archer) wrote…

I've only a few comments on the code. Let me know if you track down the theme change issue I mentioned, and I'll take a look again.

Done.

app/src/main/java/protect/card_locker/preferences/SettingsActivity.java, line 58 at r2 (raw file):

Previously, brarcher (Branden Archer) wrote…

I do not mind if the { is at the end of the line or on the next line. Kindly make it consistent in the file, though.

Done.

app/src/main/java/protect/card_locker/preferences/SettingsActivity.java, line 65 at r2 (raw file):

Previously, brarcher (Branden Archer) wrote…

Could the if/else if/ pattern be changed either to if/else if/else or if/if/? That is,

if ( /*check light theme*/) {
   // something
} else if (/*check dark theme*/) {
  //something
} else {
  //something
}
return true

or

if (/*check*) {
  //something 
  return true
}

if (/*check*/) {
  //something 
  return true
}

// last option
return true;

Done.

app/src/main/java/protect/card_locker/preferences/SettingsActivity.java, line 73 at r2 (raw file):

Previously, brarcher (Branden Archer) wrote…

If all paths end up recreating and returning true, why not do the common work at the end and return once?

Done.

brarcher

Have you ever seen the issue when launching the app when it was not running? That is, the theme was already configured, the app was swiped from recents then re-launched? Having the dark theme work well in that scenario will be more important that when the theme is being toggled, as I anticipate that if one configures their phone to be a theme it will stay that way. (Is that a valid assumption?) It may not look polished if there are transient issues when the theme changes, however having dark mode work well all other times may balance it out.

Reviewable status: 10 of 13 files reviewed, 4 unresolved discussions (waiting on @brarcher)

TheLastProject · 2019-12-06T07:11:43Z

I've only ever seen it go wrong for the very specific scenario you mentioned, and I managed to fix that so it only bugs out the about dialog now:

System in light mode, app in light mode
Launch app, enter app settings
Configure app to use dark mode
Click back to go to main app activity

brarcher

Reviewed 2 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @brarcher and @TheLastProject)

app/src/main/java/protect/card_locker/BarcodeImageWriterTask.java, line 115 at r4 (raw file):

            int bitMatrixHeight = bitMatrix.getHeight();

            // Returns: left,top,width,height enclosing rectangle of all 1 bits, or null if it is all white

I'm thinking that there still needs to be a white border around the barcode. The reason is, if the barcode goes right to the edge and it is dark theme, a detector will not be able to distinguish the boundaries of the barcode. For example, in my test setup I have an emulator and a physical phone, and I have my phone attempt to read the barcode from the emulator's screen. The phone is able to read this one:

but not this one:

With the barcode drawing changes reverted one gets the following:

which is successfully read. I recall from somewhere that barcode are intended to have a certain amount of buffer space to avoid these issues. We may not be able to remove the buffer space that the ZXing library generates.

… padding

TheLastProject · 2019-12-08T17:17:06Z

Reviewable keeps crashing whenever I try to log in on it now, so I can't leave my reply there...

Anyway, it seems you're completely right.

While I don't believe all the padding is intended because some of it is kinda absurd:

I tried changing the code to ensure that 20 pixels of padding remains, so it only removes the absurd padding:

But the 1D code there still didn't scan (but the QR code did)!

So it indeed seems some of the codes really need all the padding they are given and because doing this only for some codes and not others will very likely lead us to some edge cases with certain scanners, I've reverted the code to clean up the rectangles.

brarcher

Reviewable status: 12 of 13 files reviewed, 4 unresolved discussions (waiting on @brarcher and @TheLastProject)

app/src/main/java/protect/card_locker/BarcodeImageWriterTask.java, line 118 at r5 (raw file):

            int[] enclosingRectangle = bitMatrix.getEnclosingRectangle();

            // We add 20 pixels on all sides to ensure small intentional padding around the barcodes isn't removed

Arguably 20 pixels is arbitrary. My understanding is that the padding, or quiet zone, around a barcode should be "at least seven to ten times of the narrowest bar width in the barcode."

https://www.cognex.com/what-is/industrial-barcode-reading/1d-barcodes

I think that ZXing takes care of creating an appropriate quiet zone. My concern is that with changing the effective drawing area for the barcode that the expected quiet zone may be eroded. Can these changes be removed? Or, are they addressing a concern that I do not quite get yet?

TheLastProject · 2019-12-08T17:49:04Z

Yeah, I indeed reverted it.

brarcher

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @brarcher and @TheLastProject)

brarcher · 2019-12-09T02:16:07Z

Yeah, I indeed reverted it.

Sorry, did not see your reply initially. Sounds good!

brarcher · 2019-12-09T02:16:37Z

Thanks for adding this feature!

TheLastProject · 2019-12-09T07:44:56Z

And thank you for reviewing, testing and accepting it :)

TheLastProject and others added 8 commits December 2, 2019 15:05

Start with dark mode

9b5a731

Allow switching theme

8d3d9a2

Cleanup

061b1db

Merge branch 'master' into feature/dark_mode

a46200f

Remove another unneeded import

29e8896

Style about dialog for dark mode

4240fd5

Fixes, start with right theme

0de50e7

Merge branch 'master' into feature/dark_mode

3754243

TheLastProject added 5 commits December 4, 2019 12:45

Fix card title colour issues

4ec4baa

Merge branch 'master' into feature/dark_mode

8673405

Don't draw more than the actual barcode

5774064

Fix white border in dark theme

368a2a3

Remove useless null check

e37288b

brarcher requested changes Dec 5, 2019

View reviewed changes

Fix code style

91a4461

TheLastProject commented Dec 5, 2019

View reviewed changes

brarcher reviewed Dec 6, 2019

View reviewed changes

brarcher requested changes Dec 8, 2019

View reviewed changes

TheLastProject added 2 commits December 8, 2019 18:05

Don't remove paddings smaller than 20 pixels

08afc16

Completely removed rectangle cleanup code, 1D codes seem to need this…

61c2c0c

… padding

brarcher requested changes Dec 8, 2019

View reviewed changes

brarcher approved these changes Dec 9, 2019

View reviewed changes

brarcher merged commit acfb92e into brarcher:master Dec 9, 2019

TheLastProject deleted the feature/dark_mode branch October 24, 2020 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dark mode #322

Dark mode #322

TheLastProject commented Dec 2, 2019 •

edited by brarcher

Loading

brarcher commented Dec 4, 2019

TheLastProject commented Dec 4, 2019

brarcher left a comment

TheLastProject commented Dec 5, 2019

TheLastProject commented Dec 5, 2019

TheLastProject left a comment

brarcher left a comment

TheLastProject commented Dec 6, 2019

brarcher left a comment

TheLastProject commented Dec 8, 2019

brarcher left a comment

TheLastProject commented Dec 8, 2019

brarcher left a comment

brarcher commented Dec 9, 2019

brarcher commented Dec 9, 2019

TheLastProject commented Dec 9, 2019

Dark mode #322

Dark mode #322

Conversation

TheLastProject commented Dec 2, 2019 • edited by brarcher Loading

brarcher commented Dec 4, 2019

TheLastProject commented Dec 4, 2019

brarcher left a comment

Choose a reason for hiding this comment

TheLastProject commented Dec 5, 2019

TheLastProject commented Dec 5, 2019

TheLastProject left a comment

Choose a reason for hiding this comment

brarcher left a comment

Choose a reason for hiding this comment

TheLastProject commented Dec 6, 2019

brarcher left a comment

Choose a reason for hiding this comment

TheLastProject commented Dec 8, 2019

brarcher left a comment

Choose a reason for hiding this comment

TheLastProject commented Dec 8, 2019

brarcher left a comment

Choose a reason for hiding this comment

brarcher commented Dec 9, 2019

brarcher commented Dec 9, 2019

TheLastProject commented Dec 9, 2019

TheLastProject commented Dec 2, 2019 •

edited by brarcher

Loading