[TIMOB-24406] Android: Fix write-access for Titanium object properties

[garymathews]

• Implement writableKeys property in Kroll for @Kroll.getProperty to set accessors and fire onPropertyChanged() when object property keys are set
• Fix Titanium.UI.View.center write-access
• Fix Font write-access for ActivityIndicator, Button, Label, Switch, TextArea, TextField

TEST CASE

var win = Ti.UI.createWindow({
        title: 'TIMOB-24406',
        backgroundColor: 'gray'
    }),
    lbl = Ti.UI.createLabel({
        text: 'click me',
        font: {
            fontSize: 24
        }
    });

win.addEventListener('click', function(e) {
    // direct access to fontSize
    lbl.font.fontSize = lbl.font.fontSize + 4;
});

win.add(lbl);
win.open();

JIRA Ticket

[sgtcoolguy]

So, I think the included test case in your PR writeup should be added as a new test case (or test cases) in the PR for the test suite itself (copy https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/ti.ui.label.test.js into tests/Resources and add the new test case). It probably has to be slightly modified to be able to run in an automated way (like hang off a focus listener or something and verify that the value gets updated).

Secondly, I'm confused as to why we need the new annotation and code path to achieve this. (But don't be surprised, my children are rapidly draining my memory and ability to remember what I did for V8 here).

It looks to me like we're adding a layer of indirection for object properties like font to effectively do getters and setters. Why is that needed? do Proxy.getProperty and Proxy.onPropertyChanged not work fine for these cases? The new getter you're adding looks pretty straightforward and I'm not sure why it's necessary. The setter looks to basically unwrap the object and proxy and then just call onPropertyChanged.

More or less it looks like you're re-implementing what Proxy::onPropertyChanged and Proxy::getProperty do.

Is the general issue here that the Font object is just a generic JSObject (and not an actual Java proxy) and updating it's internal values doesn't basically fire the right calls/updates on the Java side to notify that the proxy holding the font JSObject needs to grab those new values and adjust (effectively get propertyChanged fired for font)?

[sgtcoolguy]

Hmm, that has me wondering: Did this usage ever work for Android? Doesn't seem like the V8 update would have broken this specifically. The issue would stem from the fact that a proxy has a generic JSObject property and we have no mechanism for "observing" state changes to that object to propagate up to the holding Java proxy.

One possible fix is to actually introduce a formal FontProxy.java class to hold the font structure/properties we expect, do validation, etc (like we do for Windows/TitaniumKit) and then if it's Java backed, the FontProxy class could react to property changes by propagating them up to the TiViewProxy subclass that holds it (assuming it gets a reference to the parent component somehow).

The other may be to use an approach like you are here where we mark the properties that can hold objects and implement a mechanism to notify the parent proxy whenever any property underneath gets modified (though I wonder if we can do so in a cleaner way)

[garymathews]

@sgtcoolguy Thanks for taking a look! I should have included an explanation as to why and what this PR does.

In the example Ti.UI.Label.font is a basic Object of keys and does not have a Kroll proxy to interpret property changes. And so writing to the keys directly:

label.font.fontSize = 28;

Does not let its parent proxy label know of the change (onPropertyChanged is not fired on label) and so no changes are observed. This has never worked, and the same issue exists on iOS.

I thought about creating proxies but since its just a basic Object of keys I thought it would be more convenient to handle it in Kroll and to just add accessors to each object key that fires onPropertyChanged on the parent (there's more than just Font - Point, Gradient, Dimension). The setter is the one that fixes the issue, but defining an accessor also requires a getter which is why both writableKeys_getter and writableKeys_setter exist.

But maybe proxies are the better way? I did it this way to not need to change the way the current implementation of how Font Point etc.. are used.

[sgtcoolguy]

OK, that makes more sense. So, this has actually never worked...

I'd be worried that this would effectively again just add one level of nesting to work. I'm not sure if we have any APIs that take an object which could also have an object property itself? i.e. if something like label.font.size.unit existed, I don't think this would work.

Personally I'm of the belief that something that's a formal part of our API should be modeled properly so we can do validation/etc easier as we do in TitaniumKit/Windows. That'd mean all the oddball objects we have like Font/Point/*Options

But if we want a generic JS Object nesting solution, is there any way to ensure that multiple levels of nesting would work as well?

[hansemannn]

@garymathews @sgtcoolguy Where should we go with this one? I am trying to revive our famous last page of pull requests that do not have a milestone or activity.

[lokeshchdhry]

@garymathews , Can you please resolve the conflicts. Thanks !!

[build]

	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.
🚫	Test suite crashed on iOS simulator. Please see the crash log for more details.

Generated by 🚫 dangerJS

[lokeshchdhry]

The build fails with:

[exec] src/native/Proxy.cpp: In static member function 'static void titanium::Proxy::writableKeys_setter(v8::Local<v8::Name>, v8::Local<v8::Value>, const v8::PropertyCallbackInfo<void>&)':

[exec] src/native/Proxy.cpp:563:17: error: 'unwrap' is not a member of 'titanium::Proxy'

     [exec]   Proxy* proxy = titanium::Proxy::unwrap(data->Get(0)->ToObject(isolate));

     [exec]                  ^

[exec] src/native/Proxy.cpp:581:7: error: 'useGlobalRefs' is not a member of 'titanium::JavaObject'

[exec]   if (!JavaObject::useGlobalRefs) {

     [exec]        ^

[exec] At global scope:

[exec] cc1plus: warning: unrecognized command line option "-Wno-deprecated-register"

     [exec] cc1plus: warning: unrecognized command line option "-Wno-tautological-compare"

[exec] make: *** [/Users/build/jenkins/workspace/sdk_titanium_mobile_PR-8850-RCNNV6RQIZURKN5C4SGGCE2JWKQJJKBRO6ROZ43CUBXH57V5CZ3Q/android/runtime/v8/obj/local/arm64-v8a/objs/kroll-v8/Proxy.o] Error 1

[exec] make: *** Waiting for unfinished jobs....

BUILD FAILED

/Users/build/jenkins/workspace/sdk_titanium_mobile_PR-8850-RCNNV6RQIZURKN5C4SGGCE2JWKQJJKBRO6ROZ43CUBXH57V5CZ3Q/android/build/common.xml:662: The following error occurred while executing this line:

/Users/build/jenkins/workspace/sdk_titanium_mobile_PR-8850-RCNNV6RQIZURKN5C4SGGCE2JWKQJJKBRO6ROZ43CUBXH57V5CZ3Q/android/build/common.xml:438: exec returned: 2

[longton95]

@garymathews the test case is not showing any changes when the label is clicked

[hansemannn]

Since there hasn't been much activity here for the last 3.5 years (holy moly), I would close this one, but the chances would be really too good to not take it. If Gary would be able to rebase it, I'd be happy to test it! :)

[hansemannn]

Closing for now due to inactivity and open open issues / merge conflicts that should be resolved before. Would love to have this one included after that!