Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor navbar colour #1574

Merged
merged 3 commits into from
Jan 28, 2024
Merged

Refactor navbar colour #1574

merged 3 commits into from
Jan 28, 2024

Conversation

obfusk
Copy link
Contributor

@obfusk obfusk commented Oct 12, 2023 •
edited
Loading

Follow-up for #1559 (marked draft as it is based on that to avoid a merge conflict).

@obfusk obfusk force-pushed the refactor-navbar-colour branch from 8e46847 to c5c3c2c Compare October 12, 2023 01:55
@TheLastProject
Copy link
Member

I think you may be onto something here. I'm trying to take new screenshots and sometimes the bottom bar uses the wrong colour. Every now and then it goes right. There is indeed a race condition here...

@obfusk obfusk force-pushed the refactor-navbar-colour branch from c5c3c2c to 5226bf6 Compare October 12, 2023 15:06
@obfusk
Copy link
Contributor Author

obfusk commented Oct 12, 2023

Rebased on #1559, no changes.

@obfusk
Copy link
Contributor Author

obfusk commented Oct 12, 2023

I think you may be onto something here.

Interesting. Have you tested if that still occurs with this change?

@obfusk obfusk mentioned this pull request Oct 12, 2023
Merged
@obfusk
Copy link
Contributor Author

obfusk commented Oct 12, 2023

It's ready IMO, but based on #1559 so marked as draft until that's merged. Though I guess you can merge this one and close the other instead if you prefer :)

@obfusk
Copy link
Contributor Author

obfusk commented Oct 12, 2023

We're still setting the nav bar colour twice with this code. Just in a guaranteed ordering of super.onResume() and onResume(). Could theoretically still cause a glitch depending on when it's rendered. We could refactor this to make sure we only call Utils.setNavigationBarColor() once by passing/setting a flag I guess.

@obfusk
Copy link
Contributor Author

obfusk commented Oct 12, 2023

Oh yeah, I tested this in the emulator with Android 6 and 13. And on-device with Android 13. Seemed to work as expected.

@obfusk obfusk force-pushed the refactor-navbar-colour branch 2 times, most recently from ab81eea to 6d99997 Compare October 15, 2023 14:34
@obfusk
Copy link
Contributor Author

obfusk commented Oct 15, 2023

Rebased on main.

@obfusk obfusk marked this pull request as ready for review October 15, 2023 14:45
@obfusk obfusk force-pushed the refactor-navbar-colour branch from 6d99997 to b6329e6 Compare October 15, 2023 19:33
@obfusk

This comment was marked as outdated.

Sign in to view
TheLastProject
TheLastProject requested changes Oct 16, 2023
View reviewed changes
app/src/main/java/protect/card_locker/Utils.java Show resolved Hide resolved
app/src/main/java/protect/card_locker/Utils.java Outdated
Comment on lines 673 to 675
if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.P) {
activity.findViewById(android.R.id.content).setBackgroundColor(resolveBackgroundColor(activity));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ints as that's what most of Catima users. If you want to change the standard, let's do that in another MR (and let's start documenting these things somewhere).

And you say "I tested this in the emulator with Android 6 and 13.", I'm most curious actually about the versions that no longer run this code. So, 10 and 11 and 12 and 14 too.

So this needs a lot of testing on multiple different themes (that I don't have time to do right now).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ints as that's what most of Catima users.

Does it? See above.

I'll look into testing more. No rush :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, nvm, just more testing then :)

Copy link
Member

@TheLastProject TheLastProject Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out this code is responsible for correctly applying the "Pure black background for dark theme" setting when dark theme is used in the MainActivity. Adding the version check breaks the OLED black on new Android versions (noticed on Android 14)

Strangely enough, this code isn't necessary in any other activity, only the MainActivity, what a mess...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do more testing & debugging. Hopefully this week.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it'd be best to just leave this code on all API versions with a new comment like:

// Force correct color
// Fixes OLED dark mode in MainActivity

So we at least understand better what it does. And then later debug further

@obfusk obfusk mentioned this pull request Oct 25, 2023
Open
49 tasks
@TheLastProject
Don't version-guard the background colour fix
027d153 
All Android versions seem to need this for the main screen
@TheLastProject TheLastProject merged commit c6f1e0c into CatimaLoyalty:main Jan 28, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheLastProject TheLastProject TheLastProject requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@obfusk @TheLastProject