After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 782310 - overrides: Implement Gio.ListStore[Symbol.iterator]
overrides: Implement Gio.ListStore[Symbol.iterator]
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2017-05-08 02:12 UTC by Patrick Griffis (tingping)
Modified: 2017-05-11 06:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
overrides: Implement Gio.ListStore[Symbol.iterator] (1.98 KB, patch)
2017-05-08 02:12 UTC, Patrick Griffis (tingping)
none Details | Review
overrides: Implement Gio.ListStore[Symbol.iterator] (1.99 KB, patch)
2017-05-08 02:16 UTC, Patrick Griffis (tingping)
none Details | Review
overrides: Implement Gio.ListStore.length property (1.40 KB, patch)
2017-05-08 02:57 UTC, Patrick Griffis (tingping)
rejected Details | Review
overrides: Implement Gio.ListStore[Symbol.iterator] (2.50 KB, patch)
2017-05-10 17:31 UTC, Patrick Griffis (tingping)
committed Details | Review

Description Patrick Griffis (tingping) 2017-05-08 02:12:31 UTC
Created attachment 351319 [details] [review]
overrides: Implement Gio.ListStore[Symbol.iterator]

As the title says.

I also tried to do the same for ListModel but my knowledge of JavaScript isn't quite good enough to figure out how for new GObjects implementing a GInterface can pick up that function.
Comment 1 Patrick Griffis (tingping) 2017-05-08 02:16:52 UTC
Created attachment 351322 [details] [review]
overrides: Implement Gio.ListStore[Symbol.iterator]
Comment 2 Patrick Griffis (tingping) 2017-05-08 02:26:46 UTC
Also do you know how to implement indexing the liststore directly too? (list[4], etc)
Comment 3 Patrick Griffis (tingping) 2017-05-08 02:57:50 UTC
Created attachment 351324 [details] [review]
overrides: Implement Gio.ListStore.length property

I also found out about Proxy() to override `get` to handle indexing the list which I'll look into.
Comment 4 Philip Chimento 2017-05-10 04:22:49 UTC
Review of attachment 351322 [details] [review]:

Nice, thanks!

Couple of comments; testGio.js needs to be added to Makefile-test.am as well.

::: installed-tests/js/testGio.js
@@ +15,3 @@
+    let list;
+
+    beforeAll(function () {

beforeEach() would be better here, so that there is no state shared between tests.

@@ +23,3 @@
+
+    it('ListStore iterates', function () {
+      let i = 0;

nitpick: 4 spaces indentation
Comment 5 Philip Chimento 2017-05-10 04:24:27 UTC
Review of attachment 351324 [details] [review]:

Looks reasonable, but I'm not sure it makes sense to commit this unless we do actually make ListStore behave more like an array.
Comment 6 Philip Chimento 2017-05-10 04:26:13 UTC
(In reply to Patrick Griffis (tingping) from comment #0)
> I also tried to do the same for ListModel but my knowledge of JavaScript
> isn't quite good enough to figure out how for new GObjects implementing a
> GInterface can pick up that function.

I think it's not currently possible which is a bit of an oversight in the implementation of interfaces. I'm not really intending to fix that until there are ES6 classes though.
Comment 7 Patrick Griffis (tingping) 2017-05-10 16:23:36 UTC
(In reply to Philip Chimento from comment #5)
> Review of attachment 351324 [details] [review] [review]:
> 
> Looks reasonable, but I'm not sure it makes sense to commit this unless we
> do actually make ListStore behave more like an array.

Based on what I've found searching around it isn't really possible to perfectly match array behaviour with a custom object... I do think length is still simple and useful at least though.
Comment 8 Patrick Griffis (tingping) 2017-05-10 17:31:34 UTC
Created attachment 351571 [details] [review]
overrides: Implement Gio.ListStore[Symbol.iterator]
Comment 9 Philip Chimento 2017-05-11 06:28:34 UTC
Attachment 351571 [details] pushed as 9ba3adb - overrides: Implement Gio.ListStore[Symbol.iterator]
Comment 10 Philip Chimento 2017-05-11 06:32:22 UTC
Thanks a lot for the fixup, pushed!

(In reply to Patrick Griffis (tingping) from comment #7)
> (In reply to Philip Chimento from comment #5)
> > Review of attachment 351324 [details] [review] [review] [review]:
> > 
> > Looks reasonable, but I'm not sure it makes sense to commit this unless we
> > do actually make ListStore behave more like an array.
> 
> Based on what I've found searching around it isn't really possible to
> perfectly match array behaviour with a custom object... I do think length is
> still simple and useful at least though.

After thinking about it for a while I think we should not add that. One thing that ES6 did was try to reduce the role of array-like objects that weren't quite arrays, like the arguments object; I think the JS language designers had reached a consensus that such objects were a mistake. So I think we shouldn't add more of them.

To get the length is nonetheless still very simple: store.get_n_items()
Now that it's iterable, if you want to make it into an actual array it's as simple as: [...store]

Marking the other patch as rejected accordingly.