Opened 10 years ago

Closed 7 years ago

#9801 closed defect (wontfix)

JsonRestStore: onNew() notification should pass parentInfo

Reported by: bill Owned by: Kris Zyp
Priority: high Milestone: tbd
Component: DojoX Data Version: 1.3.2
Keywords: Cc: Jean-Rubin Leonard
Blocked By: Blocking:

Description

JsonRestStore? calls onNew(item) rather than onNew(item, parentInfo).

This breaks the dijit.Tree DnD code (technically speaking, it breaks the code in tree that responds to updates to the store), since when new items appear dijit.Tree doesn't know where in the tree they belong.

Change History (11)

comment:1 Changed 10 years ago by Kris Zyp

I actually implemented parentInfo for the onNew notification earlier [19094], but then reverted it [19097] because I realized that the dojo.data API does not mandate the use of parentInfo for onNew events, and I wanted to keep the code as small as possible. Or at least according to my interpretation, it does not mandate parentInfo, although the language is very ambiguous, but I understood it to mean that parentInfo was optional, and one could do an onNew and onSet instead of single onNew.

I would be willing to reimplement this behavior, but I want to make sure it is really supposed to function this way before adding additional code (vs fixing the Tree to handle two events).

comment:2 Changed 10 years ago by bill

Cc: Jean-Rubin Leonard added; jfcunat removed

Oh I see, I didn't realize you had an onSet() event on the parent.

Well, [19097] does save about 100 bytes in your code, and I think Tree is handling the events correctly, but just very inefficiently.

When ForestStoreModel sees the onNew(item) call w/no parent, it re-runs the query to get the top level items, to find out if the new item is a top level one. That query returns a list not including the new item, and then ForestStoreModel throws away the new item since it's (apparently) not in the tree view.

When the onSet() notification comes, ForestStoreModel fetches the child item so that it can display it.

I'm sure there's some room for optimization but that's how it works now.

As to "correctness", I agree the spec is unclear. It says that parentInfo is optional but you could either interpret that to mean that the idea of parent-child relationships is store specific (and stores that don't support them don't use parentInfo), or interpret it to mean that a new child node can be reported either as one onNew(child, parent) call or as two calls, the onNew(child) and the onSet(parent).

comment:3 Changed 10 years ago by Kris Zyp

It appears to me that in the ForestStoreModel?, that an onNew event will always trigger a requery (see line 213 in _onNewItem), regardless of whether or not the parentInfo parameter is included. How would it help to include the parentInfo in the onNew event? And is it really necessary requery on every onNew event? That seems inefficient and it is also problematic for the JsonRestStore?, since it queries the server, but the new item has not been committed to the server yet.

comment:4 Changed 10 years ago by bill

Ah that's true, I got confused...

I was thinking of only requerying the top level nodes when no parent item is specified, which will work 99% of the time, although technically (now that Tree supports multi-parented nodes... ) even if an item has a parent, it might also appear at the top level. Of course applications that know that won't happen can override and optimize that function.

I think apps using Tree DnD w/JsonRestStore will need to use that caching code (ClientFilter IIRC), won't it avoid sending the query to the server altogether, or at least intermix the server's results with the items added/deleted/modified on the client (but not yet transmitted to the server)?

comment:5 Changed 10 years ago by Kris Zyp

So IIUC, passing parentInfo in onNew events won't actually change the situation (for the better). Should this be closed as wontfix or changed to adding an option to the ForestStoreModel? to not requery the top on every onNew event?

comment:6 Changed 10 years ago by bill

You can close it as wontfix if you like, but I still think it's easier for a store's client code to get one notification event (about new items) rather than two though.

As for Tree (specifically TreeStoreModel and ForestStoreModel), I'll update the comments and method names to make it clear that onNewItem() and onSetItem() are extension points that the developer is meant to override, depending on their application (and it's data). A common override will be to only requery the top level nodes when parentInfo isn't specified (hence my comment above).

comment:7 Changed 10 years ago by bill

(In [20031]) Make ForestStoreModel?.onNewItem() etc. public and make it clear that developers should override it.

Fixes #9843, refs #9801, !strict.

comment:8 Changed 10 years ago by Kris Zyp

Resolution: wontfix
Status: newclosed

comment:9 Changed 10 years ago by coa

My two cents:

  • onNew + onSet seems potentially inefficient, compared to an onNew with parentinfo?

Consider a custom (non-Tree) setup with a one-to-one relationship between items and widgets. An onNew with parentInfo means simple insertion of a new child widget.

An onNew + onSet requires going through all the children reported by onSet to first establish - has something changed at all, then: which one is new, and finally a potential insertion.

Intuitively, it would seem less efficient.

  • It also seems inconsistent when the code to remove items would be in an onDelete, whereas insertion would be "one level above" in an onSet.

Regarding the interpretation of parentInfo being optional, I would consider Bill's first interpretation ("the idea of parent-child relationships is store specific and stores that don't support them don't use parentInfo") to be more appealing.

In fact, if a store supports parent-child relationships I would advocate for parentInfo - in BOTH onNew AND onDelete. Among other things, it would make undo-tracking easier. I realize however that this would mean augmenting the data API...

comment:10 Changed 9 years ago by Kris Zyp

Resolution: wontfix
Status: closedreopened

comment:11 Changed 7 years ago by Colin Snover

Resolution: wontfix
Status: reopenedclosed

dojox/data is abandoned. Some dojox/data stores have been upgraded to use the Dojo Store API and can be found at https://github.com/kfranqueiro/dojo-smore.

Note: See TracTickets for help on using tickets.