Bug 647 - Support __defineGetter__ and __defineSetter__ for HopObjects
: Support __defineGetter__ and __defineSetter__ for HopObjects
Status: RESOLVED FIXED
: Helma
HopObject Functionality
: CVS trunk
: All All
: P1 normal
: 1.7.0
Assigned To:
:
:
:
  Show dependency treegraph
 
Reported: 2008-10-29 22:11 by
Modified: 2008-11-05 16:36 (History)


Attachments
The patch to add support for __defineGetter__ and __defineSetter__ to HopObjects (1.58 KB, patch)
2008-10-29 22:12, Juerg Lehni
Details | Diff
revised patch (1.82 KB, patch)
2008-11-05 15:06, Hannes Wallnoefer
Details | Diff
revised patch (1.65 KB, patch)
2008-11-05 16:18, Hannes Wallnoefer
Details | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-10-29 22:11:28
As Walter Krivanek pointed out on the Helma-user mailing list already,
__defineGetter__ and __defineSetter__ do currently not work for HopObject
properties.

Here a simple fix to make this to work. I am curious to hear if this might be
considered clean enough for inclusion in Helma 1.6.3 or later.
------- Comment #1 From 2008-10-29 22:12:34 -------
Created an attachment (id=100) [details]
The patch to add support for __defineGetter__ and __defineSetter__ to
HopObjects
------- Comment #2 From 2008-11-05 14:34:04 -------
The one thing that disturbs me about this patch is the implementation of
delete(). If the property is defined via ScriptableObject slot, the
ScriptableObject slot is deleted, but the Node property is left untouched.
That's not what delete() should do IMO. IIRC remember you said via IRC you
needed this behaviour for your special purposes, but I think it's somewhat
questionable to twist ECMA functionality according to personal needs. Is it ok
with you to actually delete the property in delete()?
------- Comment #3 From 2008-11-05 14:53:46 -------
I completely agree and I am fine with your proposed change. In fact I do not
need __define[GS]etter__ functionality any longer on HopObject instances, only
on HopObject prototype objects, so maybe we could consider not applying this
patch at all. After all, HopObjects also dont support dontEnum, setAttribute,
and other things like that for db fields right now.
------- Comment #4 From 2008-11-05 15:06:09 -------
Created an attachment (id=103) [details]
revised patch

Make delete() actually delete the property, plus add an explanatory comment.
------- Comment #5 From 2008-11-05 15:30:53 -------
Is it necessary to check if the node has a property before unsetting it? I am
sure that for super it is not needed, and only slows things down. So it is safe
to just call super.delete. Maybe it could be as simple as:

node.unset(name);
super.delete(name);
------- Comment #6 From 2008-11-05 16:18:04 -------
Created an attachment (id=104) [details]
revised patch

I guess you are right. New patch version with simplified delete()
------- Comment #7 From 2008-11-05 16:36:56 -------
I committed the last patch.