Opened 12 years ago

Closed 12 years ago

Last modified 10 years ago

#5724 closed defect (fixed)

Grid DojoData add/remove row broken

Reported by: haysmark Owned by: benschell
Priority: high Milestone: 1.1
Component: DojoX Grid Version: 1.0
Keywords: grid Cc:
Blocked By: Blocking:

Description

If you add a row to a Grid by calling grid.addRow, the underlying DojoData? model just says "that's nice" and doesn't call newItem on the store; effectively, it doesn't add the row. Maybe the DojoData? model is missing add and delete support?

Attachments (3)

5724.patch (9.9 KB) - added by benschell 12 years ago.
Fixes add/remove rows. Note, this patch includes fixes/enhancements from patch 5030.
dojox.grid._data.model_20080208.patch (10.9 KB) - added by Jared Jurkiewicz 12 years ago.
Updated patch for style issues.
5724-2.patch (473 bytes) - added by benschell 12 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 12 years ago by benschell

Owner: changed from sorvell to benschell

I'm taking a stab at a whole bunch of Grid bugs. Re-assigning.

comment:2 Changed 12 years ago by benschell

Status: newassigned

comment:3 Changed 12 years ago by Adam Peller

Component: DijitDojoX Grid

Changed 12 years ago by benschell

Attachment: 5724.patch added

Fixes add/remove rows. Note, this patch includes fixes/enhancements from patch 5030.

comment:4 Changed 12 years ago by Jared Jurkiewicz

Reviewed. There are some minor stylistic things to fix (I'll go ahead and fix them). The logic seems reasonable for the actions.

Tested on:

FireFox? 2.0.0.11: PASSED

IE 6: PASSED

Opera 9.2: PASSED

Safari B3: PASSED

Seamonkey 1.1.2: PASSED

Changed 12 years ago by Jared Jurkiewicz

Updated patch for style issues.

comment:5 Changed 12 years ago by Jared Jurkiewicz

Resolution: fixed
Status: assignedclosed

(In [12322]) Updated to the DojoData? model. fixes #5724 !strict

comment:6 Changed 12 years ago by guest

Resolution: fixed
Status: closedreopened

Remove function passes arguments, not inRowIndexes to _removeItems on line 683. Should be:

this._removeItems(inRowIndexes);

CLA Manninen

comment:7 Changed 12 years ago by guest

To reproduce the abovementioned issue see: test_dojo_data_edit. "Remove (Store)" is broken. The first item gets removed instead of the selected ones.

comment:8 in reply to:  7 Changed 12 years ago by benschell

Replying to guest:

To reproduce the abovementioned issue see: test_dojo_data_edit. "Remove (Store)" is broken. The first item gets removed instead of the selected ones.

Actually, this is as intended. That button removes the first item in the model directly from the store. This is in contrast to the other Remove button, which removes the selected item using the Grid's removeSelectedRows function. More specifically, the Remove (Store) button calls this:

removeItem = function() {
	// Removes the first item in the model from the store
	// Grid should reflect removal of the first item and items should be re-indexed
	jsonStore.deleteItem(dataModel.data[0].__dojo_data_item);
}

Line 683 in model.js does indeed look odd as indicated above, however this is working properly. Items removed from the store (by using the "Remove (Store)" button) are removed from both Grids on the indicated test page, which is the expected behavior. I'm not seeing where the bug is.

comment:9 Changed 12 years ago by guest

I still argue that there's a bug here. "Remove (Store)" was maybe my mistake, but see "Remove". On the the page I mentioned the grid rows are in the beginning:

0 Afrika
1 Egypt
2 Kenya
3 Nairobi
4 Mombasa

Select 1 & 3 and click Remove - what you have now is:

0 Egypt
1 Kenya
2 Nairobi
3 Mombasa

Only Africa is gone.

Try also 1) refreshing the page, 2) selecting 1 & 3 and 3) clicking "Remove (Store)". You get "Removal [ [ 0]]" twice on FireBug? console and once again only the first item is gone.

comment:10 Changed 12 years ago by benschell

The second item you listed is still as expected. The "Remove (Store)" button has nothing to do with the selected rows in the table, as it is interacting directly with the data store (which has no knowledge of the rows selected in the Grid). "Removal 0?" is printed twice, as each Grid receives the notification from the data store and removes the appropriate row.

Hoewever, I'll agree that the first behavior (selecting two rows and selecting 'Remove' only results in the first row being removed) is a bug, and I'll look into it. I think my folly here was not testing deleting multiple rows at once, as you pointed out.

comment:11 Changed 12 years ago by benschell

Oh my, I broke Trac. I meant "Removal [ [ 0]]"

Changed 12 years ago by benschell

Attachment: 5724-2.patch added

comment:12 Changed 12 years ago by benschell

As suggested, this patch fixes the problem. Now, selecting multiple rows to be deleted will result in the correct rows being deleted from the current Grid.

comment:13 Changed 12 years ago by Jared Jurkiewicz

Resolution: fixed
Status: reopenedclosed

(In [12739]) Minor tweak to #5724 fix. Provided by Ben Schell. fixes #5724 !strict

Note: See TracTickets for help on using tickets.