titanium-sdk icon indicating copy to clipboard operation
titanium-sdk copied to clipboard

[Breaking change] Android: Ti.UI.Color parity

Open drauggres opened this issue 2 years ago • 7 comments

  • [x] Ti.UI.fetchSemanticColor returns Ti.UI.Color
  • [x] Support Ti.UI.Color instances for components properties
  • [x] Automatically change color value on dark/light mode switch

This PR includes commit from #13267 and #13265 Fix #13272

Read this before merging.

drauggres avatar Feb 17 '22 12:02 drauggres

Fails
:no_entry_sign: Test reports missing for Android Main. This indicates that a build failed or the test app crashed
Warnings
:warning: There is no linked JIRA ticket in the PR body. Please include the URL of the relevant JIRA ticket. If you need to, you may file a ticket on JIRA
Messages
:book: :fist: The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
:book:

:white_check_mark: All tests are passing Nice one! All 14752 tests are passing. (There are 934 skipped tests not included in that total)

:book: :tada: Another contribution from our awesome community member, drauggres! Thanks again for helping us make Titanium SDK better. :thumbsup:

Generated by :no_entry_sign: dangerJS against 7fc7d01c4ac0c2362dd70e9e7f72a32b0db19984

build avatar Feb 17 '22 13:02 build

FYI: The reason we didn't add Ti.UI.Color proxy support to Android is because it would be a HUGE breaking-change to existing modules since many of them blindly cast a color property's value to (String), which would cause a casting exception.

For example, your changes would break the following... https://github.com/appcelerator-modules/ti.map/blob/master/android/src/ti/map/CircleProxy.java#L163 https://github.com/appcelerator-modules/ti.map/blob/master/android/src/ti/map/PolygonProxy.java#L122 https://github.com/appcelerator-modules/ti.admob/blob/master/android/src/ti/admob/AdmobView.java#L116 https://github.com/appcelerator-modules/titanium-web-dialog/blob/master/android/src/ti/webdialog/Utils.java#L68

The backward compatible solution that we implemented embeds both dark/light color values into a single string when calling fetchSemanticColor(). That's the "ti.semantic.color:dark=value;light=value" string format I'm sure you've seen. That's the color string you want to assign to a view proxy's properties so that it will support dark/light theme switching... and this definitely works.

Anyways, that's my 2 cents. This March, you guys can handle this anyway you want. If you want to move forward with this, then my recommendation is to do this in a new Titanium 11.0.0 release and force everyone to rebuild their modules.

jquick-axway avatar Feb 17 '22 20:02 jquick-axway

Thanks @jquick-axway, I'm glad you are still with us.

drauggres avatar Feb 17 '22 21:02 drauggres

Very impressive PR! So thrilled to get this reviewed and merged when TiDev can take over.

hansemannn avatar Feb 20 '22 14:02 hansemannn

@drauggres A few merge conflicts to resolve here, but no time pressure as it's for 11.0.0 (targeted for September) anyways.

hansemannn avatar Mar 27 '22 13:03 hansemannn

A few merge conflicts to resolve here, but no time pressure as it's for 11.0.0 (targeted for September) anyways.

This happened bacause this PR had to include commits from #13267 and #13265 and they were "merged" with rebase, i.e. not really merged (in git terms) but copies of the commits were created.

I rebased my branch, should be fine now.

drauggres avatar Mar 27 '22 13:03 drauggres

@m1ga And this one as well

hansemannn avatar Dec 23 '22 13:12 hansemannn