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.

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

Android: fix Ti.UI.overrideUserInterfaceStyle #13267

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

drauggres
Copy link
Contributor

@drauggres drauggres commented Feb 10, 2022

JIRA: https://jira.appcelerator.org/browse/TIMOB-28563
fix #13266

Known issues:

  • TiGradientDrawable: doesn't have a proxy reference
  • TiAnimationBuilder: proxy is assinged in start() methond, i.e. after parameters calculation.

In both cases classes could be refactored, but I'm not going to do it now.
As simple workaround it is possible to use tiApp.getRootOrCurrentActivity() instead of TiApplication.getInstance(). In most cases this will give the desired result, but is not really correct, since it's possible that a proxy will have different Activity from current.

If someone will be thinking about merging this, please consider refactoring classes mentioned above or at least creating issues, so this don't get lost.

@drauggres
Copy link
Contributor Author

We need to use Activity (its Configuration) stored in a Proxy when we resolve colors from a semantic-name string.
Now in the SDK we use "global" Application context instead, which is wrong.

I changed behaviour for some basic cases as a PoC.
The same must be done for each Proxy and each color-related property (will try to finish next week).

@hansemannn

@build
Copy link
Contributor

build commented Feb 10, 2022

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 19694 tests are passing.
(There are 1157 skipped tests not included in that total)

📖 🎉 Another contribution from our awesome community member, drauggres! Thanks again for helping us make Titanium SDK better. 👍
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS against 315d018

@drauggres
Copy link
Contributor Author

Apparently, failed tests are not related to changes in this PR.
Same for example here: #13260

@hansemannn
Copy link
Collaborator

Thats pretty cool, thank you @drauggres! I did not expect to see any progress regarding this, wow! Will try to test in over the next weeks!

@drauggres drauggres force-pushed the bug/overrideUserInterfaceStyle branch from 728e3a0 to 315d018 Compare February 15, 2022 12:46
@drauggres drauggres marked this pull request as ready for review February 15, 2022 12:57
@hansemannn
Copy link
Collaborator

hansemannn commented Mar 21, 2022

@drauggres Can you please pull from master again? The build will then succeed :)

@hansemannn
Copy link
Collaborator

There is one new merge conflict because of the recent separator-color changes.

fix Ti.UI.overrideUserInterfaceStyle

fix TIMOB-28563
@drauggres drauggres force-pushed the bug/overrideUserInterfaceStyle branch from 56e5057 to 8608c6a Compare March 21, 2022 14:43
@drauggres
Copy link
Contributor Author

There is one new merge conflict because of the recent separator-color changes.

rebased and fixed

@hansemannn
Copy link
Collaborator

Thank you! But the PR looks much larger now. And kind of mixed with #13273, is that possible? Due to the force-push, this is not clearly visible anymore.

@drauggres
Copy link
Contributor Author

But the PR looks much larger now.

It is not. This is what it has before update : 315d018. And this is what it has now: 8608c6a

And kind of mixed with #13273, is that possible?

It is other way around: #13273 includes this PR (and #13265). I mentioned it in the description.

@drauggres
Copy link
Contributor Author

In this PR I had to update every single line where color is assigned and add an Activity reference.
In #13273 I had to update the same lines, and add support for Ti.UI.Color instance in arguments.

@hansemannn
Copy link
Collaborator

Understood, thank you. Didn't mean to be rude here 🧐.

@drauggres
Copy link
Contributor Author

Understood, thank you. Didn't mean to be rude here monocle_face.

What? When? You wasn't rude. Was I?
I just tried to state the facts straight.

@hansemannn
Copy link
Collaborator

Looks fine from my end. @m1ga ?

@m1ga
Copy link
Contributor

m1ga commented Mar 25, 2022

It adds another function to the helper so parseColor stays the same in case modules use it, so we are safe there 👍
And most of the other changes are just using the new method. All tests passed so that awesome too 👍

I will checkout the PR later today and give it a test run

@m1ga
Copy link
Contributor

m1ga commented Mar 25, 2022

@hansemannn

Using the test app from the ticket I've tested:

  • starting in dark or light mode -> shows correct mode ✔️
  • overriding the mode works fine ✔️
  • removing the override shows the correct system mode again ✔️

Tested my existing app ✔️ :

  • no issues in showing the correct colors
  • changing the system dark/light mode updated the app without any issue
  • no issues using different android modules (like FCM, ti.animation, map)

Side note:
not having a semantic.json file BUT using label colors will result in some hiccups (#13266 (comment)) but it is not related to this PR and is the same in the current GA too. It was of course my fault since I didn't include the file. Maybe we can investigate in a different issue if there is a way to prevent that. Adding it as a default file (#13241) should be one step closer 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: Overriding user interface style does not work properly
4 participants