romgrk / node-gtk

GTK+ bindings for NodeJS (via GObject introspection)
MIT License
494 stars 41 forks source link

fix(GObject): don't set gobject properties from dashed property names #279

Closed peat-psuwit closed 3 years ago

peat-psuwit commented 3 years ago

Restore dashed property check in GObjectFallbackPropertySetter() which is removed in commit 03ff4ad ("fix(GObject): dont throw/catch when setting properties"), and add a test case to ensure the correctness in this case.

While we're at it, remove a stale reference to exception inside GObjectFallbackPropertySetter() in a separate commit.

romgrk commented 3 years ago

Yeah sorry I touched that part recently because any prop set would trigger errors in ndb, quite annoying for debugging. I removed the dash-to-camel part because I figured the use case of setting JS dash-props and intending them as JS props and not GObject props wouldn't exist, and I'd rather have less code if possible. Any reason why you'd rather have it restored?

Also I'm not sure why GHA doesn't run here :/ But anyway we can merge in master and fix if something arises, I don't expect this to fail.

peat-psuwit commented 3 years ago
  1. It's about consistency. The code in master currently does this:

    > const src = Gst.ElementFactory.make('videotestsrc', 'src')
    undefined
    > src['is-live']
    undefined
    > src['is-live'] = true
    true
    > src['is-live']
    undefined
    > 

    So, to be consistent, the code that check this in the get path would have to be removed too, but

  2. Support dashed property name too will require a bit of code to properly validate the property name. Say, a GObject object has a property named is-offset-valid. The following should be valid:

    • obj.isOffsetValid
    • obj['is-offset-valid']

But not these:

If you want this, I would try to implement it. But I would argue that it's unneeded.

romgrk commented 3 years ago

Ok, sounds good.