haxeui-core icon indicating copy to clipboard operation
haxeui-core copied to clipboard

Binding variable

Open Shallowmallow opened this issue 3 years ago • 6 comments

Expected Behavior

I try binding a variable

<vbox>
<hbox>
    <vbox id="alphabet" />
    <hbox id="letters">
        <label id="upper_case" text="upper"/>
        <label id="lower_case" text="lower"/>
    </hbox>
</hbox>

</vbox>

@:bind(lower_case.hidden) public var hasLowerCase:Bool = false;

It should work

Current Behavior

In javascript , it generates

,get_hasLowerCase: function() {
		var c = this.findComponent("lower_case",haxe_ui_core_Component);
		if(c == null) {
			console.log("haxe/ui/macros/Macros.hx:265:","WARNING: no child component found: " + "lower_case");
			return haxe_ui_util_Variant.toBool(haxe_ui_util_Variant.fromDynamic(c.get_hidden()));
		}
		var fieldIndex = Type.getInstanceFields(js_Boot.getClass(c)).indexOf("get_" + "hidden");
		if(fieldIndex == -1) {
			console.log("haxe/ui/macros/Macros.hx:270:","WARNING: no component getter found: " + "hidden");
			return haxe_ui_util_Variant.toBool(haxe_ui_util_Variant.fromDynamic(c.get_hidden()));
		}
		return haxe_ui_util_Variant.toBool(haxe_ui_util_Variant.fromDynamic(c.get_hidden()));
	}

It doesn't find c.

Possible Problems

There are two problems with the generated code

  • for some reason it doesn't find the component
  • but also
if(c == null) {
			console.log("haxe/ui/macros/Macros.hx:265:","WARNING: no child component found: " + "lower_case");
			return haxe_ui_util_Variant.toBool(haxe_ui_util_Variant.fromDynamic(c.get_hidden()));
		}

You cannot return c.get_hidden() if c is null;

Context

I was just testing binding variables. No big deal if it doesn't work

Shallowmallow avatar Sep 18 '22 21:09 Shallowmallow

So that logic looks iffy any way, as you said "if c is null, do something with c"... thats always going to break, weird its null in the first place though... Can you attach the sample project? (Just so i can make sure the code is "sensible", which it looks to be from the snippets)

ianharrigan avatar Sep 19 '22 07:09 ianharrigan

Also, im not sure all the fieldIndex stuff makes sense either, i mean, it'll fail to compile of get_hidden didnt exist

ianharrigan avatar Sep 19 '22 07:09 ianharrigan

So, ive just written a test project and it all seems to work fine. So yeah a runnable minimal repo is needed. That said, i really dont like that generated code at all so going to fix it anyway.

ianharrigan avatar Sep 19 '22 07:09 ianharrigan

I'll try to make a minimal repo today, (I just tested in my code) , but as you said, the generated code made no sense, so I decided to post directly.

Shallowmallow avatar Sep 19 '22 07:09 Shallowmallow

OK, i changed the macro that build the code, now it builds something like:

	,get_hasLowerCase: function() {
		var c = this.findComponent("lower_case",haxe_ui_components_Label);
		if(c == null) {
			console.log("haxe/ui/macros/Macros.hx:278:","WARNING: no child component found: " + "lower_case");
			return haxe_ui_util_Variant.toBool(haxe_ui_util_Variant.fromDynamic(null));
		}
		return haxe_ui_util_Variant.toBool(haxe_ui_util_Variant.fromDynamic(c.get_hidden()));
	}
	,set_hasLowerCase: function(value) {
		if(value != this.get_hasLowerCase()) {
			var c = this.findComponent("lower_case",haxe_ui_components_Label);
			if(c == null) {
				console.log("haxe/ui/macros/Macros.hx:290:","WARNING: no child component found: " + "lower_case");
				return value;
			}
			c.set_hidden(haxe_ui_util_Variant.toBool(haxe_ui_util_Variant.fromDynamic(value)));
		}
		return value;
	}

Which makes alot more sense:

  • if c is null in the getter, it will return a null, not try to get the prop from it (which would end in an NPE)
  • that fieldIndex stuff is gone - no idea what the point of that ever was tbh, i mean, if the field didnt exist, it would fail on compilation
  • the findComponent call now uses the correct component class type, previously it was always "haxe.ui.core.Component"... which would have been fine for things like text, hidden, etc (ie, properties that are in all components), but would have failed for things like "max" on a slider.

Seems like all of that was very old code!

Cheers, Ian

ianharrigan avatar Sep 19 '22 08:09 ianharrigan

Addendum:

ive also just removed (some) of the variant stuff. Also not exactly sure why that was there, so the generated code is now:

	,get_hasLowerCase: function() {
		var c = this.findComponent("lower_case",haxe_ui_components_Label);
		if(c == null) {
			console.log("haxe/ui/macros/Macros.hx:278:","WARNING: no child component found: " + "lower_case");
			return haxe_ui_util_Variant.toBool(haxe_ui_util_Variant.fromDynamic(null));
		}
		return c.get_hidden();
	}
	,set_hasLowerCase: function(value) {
		if(value != this.get_hasLowerCase()) {
			var c = this.findComponent("lower_case",haxe_ui_components_Label);
			if(c == null) {
				console.log("haxe/ui/macros/Macros.hx:290:","WARNING: no child component found: " + "lower_case");
				return value;
			}
			c.set_hidden(value);
		}
		return value;
	}

ianharrigan avatar Sep 19 '22 08:09 ianharrigan

(Works now :) )

Shallowmallow avatar Dec 23 '23 18:12 Shallowmallow