Fix white on white icons

[TheLastProject]

Fixes #300

---

This change is 

[brarcher]

Reviewed 26 of 26 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @TheLastProject)

a discussion (no related file):
Does there need to be a separate copy if the icons for the different colors? I believe that the color can be adjusted programmatically. For example:

https://stackoverflow.com/questions/11376516/change-drawable-color-programmatically

---

---

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java, line 172 at r1 (raw file):

        // If the background is very bright, we should use dark icons
        backgroundNeedsDarkIcons = (ColorUtils.calculateLuminance(backgroundHeaderColor) > 0.5);

Could you make 0.5 a const instead of a magic number? Not sure what to call it. Maybe LUMINANCE_MIDPOINT, or something communicating half of the possible brightness?

[TheLastProject]

Reviewable status: 25 of 26 files reviewed, 2 unresolved discussions (waiting on @brarcher)

a discussion (no related file):

Previously, brarcher (Branden Archer) wrote…

Does there need to be a separate copy if the icons for the different colors? I believe that the color can be adjusted programmatically. For example:

https://stackoverflow.com/questions/11376516/change-drawable-color-programmatically

I just took a look into those, but it seems of all the methods the lowest required SDK level would be 21, which I believe would mean dropping support for anything lower than Android 5.0? I'd love to be wrong here though...

---

---

app/src/main/java/protect/card_locker/LoyaltyCardViewActivity.java, line 172 at r1 (raw file):

Previously, brarcher (Branden Archer) wrote…

Could you make 0.5 a const instead of a magic number? Not sure what to call it. Maybe LUMINANCE_MIDPOINT, or something communicating half of the possible brightness?

Done.

[brarcher]

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TheLastProject)

a discussion (no related file):

Previously, TheLastProject (Sylvia van Os) wrote…

I just took a look into those, but it seems of all the methods the lowest required SDK level would be 21, which I believe would mean dropping support for anything lower than Android 5.0? I'd love to be wrong here though...

Looking at the Android devices that install the app from the Play store, less than 1% are on Android 4.4 or lower. I'm fine with updating the minimum SDK to 21. Devices lower than that version will still have the current app version available, but would not longer receive updates.

As an alternative, the code which accesses the new APIs could be guarded with code like the following:

import android.os.Build;

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
     // Do color adjustments here
}

This would mean that older devices are still supported, though this bug will always affect the device. Users are able to work around the bug by selecting a darker color, so I do not think that the issue is critical.

I'm fine with either option. Moving the min SDK up may make maintenance a little simpler.

---

[TheLastProject]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @brarcher)

a discussion (no related file):

Previously, brarcher (Branden Archer) wrote…

Looking at the Android devices that install the app from the Play store, less than 1% are on Android 4.4 or lower. I'm fine with updating the minimum SDK to 21. Devices lower than that version will still have the current app version available, but would not longer receive updates.

As an alternative, the code which accesses the new APIs could be guarded with code like the following:

import android.os.Build;

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
     // Do color adjustments here
}

This would mean that older devices are still supported, though this bug will always affect the device. Users are able to work around the bug by selecting a darker color, so I do not think that the issue is critical.

I'm fine with either option. Moving the min SDK up may make maintenance a little simpler.

I think I misunderstood a warning Android Studio gave me earlier but this time I didn't seen to get a warning that it needed minSdk 21. Well, done :)

---

[TheLastProject]

Also I had to include ic_arrow_back_white.png to prevent it from falling back on some really weird icon for the white variant.

[brarcher]

Reviewed 5 of 30 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TheLastProject)

a discussion (no related file):

Previously, TheLastProject (Sylvia van Os) wrote…

I think I misunderstood a warning Android Studio gave me earlier but this time I didn't seen to get a warning that it needed minSdk 21. Well, done :)

Testing this out on an emulator, I see that it works fine. I'm curious if you are interested in also adjusting the color of the title text and the notification bar icons also? That is, for the following example:

the menu icons are much better, although the title text and notification icons are still hard to see. If you want to limit this pull request to the menu icons, that is fine. If you are interested in looking at the other areas for improvement, I can hold off on merging in the request. Just let me know. Here is a reference for adjusting the color programmatically:

https://stackoverflow.com/questions/33316668/how-to-change-the-status-bar-notification-icons-color-tint-in-android-marshmal

---

[TheLastProject]

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @brarcher)

a discussion (no related file):

Previously, brarcher (Branden Archer) wrote…

Testing this out on an emulator, I see that it works fine. I'm curious if you are interested in also adjusting the color of the title text and the notification bar icons also? That is, for the following example:

the menu icons are much better, although the title text and notification icons are still hard to see. If you want to limit this pull request to the menu icons, that is fine. If you are interested in looking at the other areas for improvement, I can hold off on merging in the request. Just let me know. Here is a reference for adjusting the color programmatically:

https://stackoverflow.com/questions/33316668/how-to-change-the-status-bar-notification-icons-color-tint-in-android-marshmal

Done.

---

[TheLastProject]

Also threw in c0d1b44 to help protect users a bit against low to no difference between background header and text color:

I don't think we can ever make that situation really pretty, but this is at least somewhat visible.

[brarcher]

Thanks for the extra changes. Looks good

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[brarcher]

Thanks for the fix!