Opened 12 years ago

Closed 8 years ago

#4122 closed enhancement (wontfix)

dijit.WidgetSet: support array interface

Reported by: alex Owned by: bill
Priority: high Milestone: 2.0
Component: Dijit Version: 0.9
Keywords: 4alex Cc: alex
Blocked By: Blocking:

Description (last modified by bill)

The methods on dijit.WidgetSet (and by association, dijit.registry) appear to surface array-like semantics (forEach, filter, etc.), however these methods do not take a scope parameter and do not return an array-like object. Given that many other bread-and-butter Dojo interfaces respect array-like semantics (sometimes going to great length to do so), these methods should probably be re-designed and a toArray() method (or better, basic array behavior on the base class itself) should be added to the class.

Change History (12)

comment:1 Changed 12 years ago by bill

Owner: changed from bill to alex

For efficiency reasons WidgetSet? holds a hash rather than an array, since the widgets are typically accessed by id.

toArray() method was left out on purpose, to prevent users from doing something inefficient like dojo.filter(widgetSet.toArray(), ...) rather than widgetSet.filter(..).

I guess the return value from byClass() or filter() could be an array, although I want to do things like

dijit.byClass("Button").forEach(...)

which means that we need to return a souped-up array like NodeList? does, not a plain []. There should probably be support for that in dojo.

Talked to Alex about this. He said he'll make a backwards-compatible patch to support stuff like this and then (if it looks OK) I'll add to 1.0.

comment:2 Changed 12 years ago by bill

PS: oh Alex said that he wants a toArray() function for debugging output. I don't mind adding it for that reason but let's name it _dumpToArray() to make it clear that people shouldn't be using it in general.

comment:3 Changed 12 years ago by bill

Keywords: 4alex added

comment:4 Changed 11 years ago by bill

Milestone: 1.12.0

Too late to make an API change (or even addition) for Dijit 1.1... bumping.

comment:5 Changed 10 years ago by david

Bumping this up for discussion again...

There doesn't seem to be a good way to get a set of widgets on the page. It seems to me that a toArray method is necessary. Sure, you don't want to use it for general purpose things, but it sure makes life easier. It also doesn't make sense to me to partially implement things like forEach if you aren't giving people an easy way to get at the data.

While I'm at it, a dijit equivelent to dojo.query would be mighty nice. Quick, how do I get all the dijit.form.* dijits under a given dijit.form.Form? (Current answer: {{{dojo.filter(myFormDijit.getDescendants(), "return item.declaredClass.indexOf('dijit.form.')==0")}}}) Maybe I should make this a new feature request?

comment:6 Changed 10 years ago by bill

Description: modified (diff)
Summary: dijit.WidgetSet methods broken or mis-designeddijit.WidgetSet: support array interface
Type: defectenhancement

I'm not sure what this means:

It also doesn't make sense to me to partially implement things like forEach if you aren't giving people an easy way to get at the data.

If you are talking about forEach() not taking a scope variable, that could definitely be added. If you are saying that you just want an array of output, not sure how that's related to forEach().

Regarding dojo.query() for dijits, I have heard discussion about that although, yes, it shouldn't be part of this ticket. Should make a new ticket... although probably the idea is too vague right now to even make a ticket....

Need to think about syntax etc. Would it be something like dijit.query(".foo dijit.form.Button") to find all button widgets with an ancestor DOM node with class=foo ? That syntax is no good b/c it's ambiguous. Anyway, think about what kind of API you would like. Think about whether it needs to work on DOM nodes or just widgets, and whether it needs to do things like querying on widget attributes (ex: find all Button widgets with name=foo, or BorderContainer widgets with design=headline)

comment:7 Changed 10 years ago by david

I'm not sure what this means:

It also doesn't make sense to me to partially implement things like forEach if you >> aren't giving people an easy way to get at the data.

What I was trying to point out is that since we don't expose the widgets as an array, which would allow you to pass it into all the handy dojo array functions, what we do offer should be more "full-featured." Does that make more sense?

If we really feel that providing a toArray function isn't a good idea, there needs to at least be a way to get at an item by something other than id. I suppose you could mimic the DOM and have an item method, but that'd probably be less performant, especially if we provide a length property (should I file a bug for that?). Basically, right now WidgetSets are very mysterious to me. You can't really see what's in them but you can iterate over them somehow. I don't know, they just don't feel quite right at the moment.

comment:8 Changed 10 years ago by dante

see #9290, it implements about half of this functionality with a FIXME note for every() and some() ... patch is back-compat and adds a few new APIs. Can probably close this after commit and open new ticket filling in the remaining gaps.

comment:9 Changed 9 years ago by bill

Reconsidering this, I think I'm in favor of converting WidgetSet to be an extended array (similar to NodeList).

The downside is that the primary WidgetSet, dijit.registry, will often contain many widgets, and when a widget is destroyed it's an O(n) operation to remove it. Maybe there's a fancy way to avoid that by maintaining an index mapping widget id --> array index but that seems to lead to another O(n) problem maintaining the index.

Obviously dijit.byId() still needs to operate on a hash, to be fast, so we would still need to maintain a hash of all widgets; not sure if that would be part of WidgetSet or separate.

comment:10 Changed 9 years ago by bill

Actually, it seems like the whole registry hash is unnecessary. Since in practice every widget sets id on either it's root node or (in the case of form widgets) it's focus node, dijit.byId could be implemented as:

dijit.byId(id) = function(id){
   return dijit.getEnclosingWidget(dojo.byId(id));
}

(It could alternately do a dojo.query("[widgetId=" + id + "]) but I think that would be O(n) not O(1))

Similarly, forEach() etc. could be implemented as:

dijit.registry.forEach(func) = function(func){
   return dojo.query("[widgetId]").map(dojo.byNode);
}

It would be slower, but not sure how important the speed difference would be. Also, it might fail in some corner cases, when the widget wasn't added to the document, but hopefully that's not an issue in practice.

The WidgetSet's class itself would cease to be used in dijit, although I suppose that for back-compat reasons we need to support it until 2.0.

comment:11 Changed 8 years ago by Chris Mitchell

Owner: changed from alex to bill

comment:12 Changed 8 years ago by bill

Resolution: wontfix
Status: newclosed

Closing this ticket. At some point toArray() got added and I've actually deprecated the forEach() etc. type functions and WidgetSet itself in [26052], expecting apps instead to call array.forEach(registry.toArray(), ...).

There are many places in dijit and dojo where it would be convention to be able to chain, for example:

myWidget.getChildren().forEach(...)

Might be nice to have a general solution for that (i.e. an extended array class like NodeList), but anyway there's no special consideration for the registry.

Note: See TracTickets for help on using tickets.