Page MenuHomeWildfire Games

Don't pretend we implement a JS constructor for GUIObject.

Authored by elexis on May 28 2019, 6:45 PM.


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
rP22530: Delete unused broken JSI_IGUIObject::construct.

The JSGUI Interface defines a constructor for GUIObject from JS. This constructor is never used in js code, and only supports cloning the JS part of a C++ GUI object (since the private data is the same c++ object).
This deletes that and instead errors out, if we ever properly implement this constructor, it can be changed.

Test Plan


Diff Detail

rP 0 A.D. Public Repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.May 28 2019, 6:45 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build:

wraitii added a subscriber: Itms.May 28 2019, 7:58 PM

@Itms Possibly of interest to you.

Stan added a subscriber: Stan.May 28 2019, 7:59 PM
Stan added inline comments.
616 ↗(On Diff #8193)

not implemented ?

Do we have places where the removed code might be useful? Why it was added?

elexis added a subscriber: elexis.Jun 2 2019, 1:43 AM

Some overlap with D1699

Do we have places where the removed code might be useful? Why it was added?

It was added because it could be, I guess. It's there since 2004, and as far as I can tell hasn't changed much since (SM updates only, basically). The purpose is to have JS code call "new GUIObject", but since it needs to take another GUI object as an argument it seems to be a copy constructor? Only it doesn't copy the c++ object. I'm not sure if it was ever used but it does not seem to be now.

In D1933#80581, @elexis wrote:

Some overlap with D1699

Indeed, forgot that.

elexis commandeered this revision.Jul 23 2019, 3:24 AM
elexis added a reviewer: wraitii.

The function should be deleted altogehter, then new GUIObject() will complain without writing code for it.
D1699 has some more information, but I will split that revision into smaller diffs, including this one.

Do we have places where the removed code might be useful? Why it was added?

It was added in rP666, it can only be a mistake, because one entirely cannot pass a IGUIObject C++ class instance pointer via JS, nor construct that in JS, nor would make it sense to create such an object.
If someone wanted to have a use case to dynamically insert GUIObjects, it would at least have to be a constructor depending on GUI type, i.e. new GUIDropDown etc..
(That use case is unlikely, right now we specify everything in XML and build the code refering to that, but if someone wants to go for that new code, they should it do it in a way that doesn't break in various ways and allows the UI to segfault for no benefit.)

74 ↗(On Diff #8193)

These commens are more confusing than helping.

  • There are no secrets in the revision log from rP666+ I could find, but this code is only here to make this function work correctly (not returning false for methods), which doesnt need a comment.
  • The *must* part is invalidated by the later commit which introduced the early return for e

"toJSON" doesnt exist, although it's often defined for JSObjects ( and I guess Yves added it for a reason (in rP13238).

If a JSPropertyOp or JSStrictPropertyOp does nothing and returns true, then property get, set, or add is unaffected. It proceeds as normal.

So in theory toJSON and prototype would return something defined by SM.
constructor actually does return a JSNative.

One can look into splitting the getter into multiple functions, but not here.

616 ↗(On Diff #8193)

delete function completely, this error is redundant

This revision was not accepted when it landed; it landed in state Needs Review.Jul 23 2019, 3:24 AM
This revision was automatically updated to reflect the committed changes.