PDA

View Full Version : [DEFER-82] [2.x/3.x] GridView.refreshRow implementation is seriously broken



grzegorz.borkowski
11 Mar 2009, 5:31 AM
I don't know why refreshRow in GridView is implemented in this way that it first creates new row, and then removes actual one (instead of just updating the existing one). There must have been some reasons for such strange approach, I believe. But it looks rather like a nasty workaround, and it has significant negative impact.
I'm trying to build some proof-of-concept application with Ext. I'm heavily using editor grids with non-standard selection model and with row expanders.
I spent long time trying to fix many errors with selection model, coused by refreshRow implementation. After each row change, selection model gets lost, because the row that was previously selected suddenly vanishes. This requires some new hacks to fix it.
Then I have expanders in rows, containing other forms, tabbed panes etc. So user expands the row, edits the form, spend long time on it, and then edit the data in row itself in the grid. After this all that was done in the expander vanishes, because refreshRow method actually removed old record with its expander body! This is horror. I've spent countless hours trying to fix it, but that's now very fragile still.
The only reliable way would be to override the refreshRow method in GridView, and implement it correctly, without recreation of the row. But I believe it would be better if it is simply reimplemented and fixed in Ext distribution, say in Ext 3.

Condor
11 Mar 2009, 6:47 AM
Here (http://www.extjs.com/forum/showthread.php?t=51734) is another problem caused by the fact that refreshRow replaces the row.

grzegorz.borkowski
11 Mar 2009, 6:52 AM
Yes, and that's another example that hacky refreshRow implementation makes you have to use other hacks/workarounds to make things working.

Animal
12 Mar 2009, 12:55 AM
A first stab at a fix:



Ext.override(Ext.grid.GridView, {
// private
refreshRow : function(record){
var ds = this.ds, rowIndex;
if(typeof record == 'number'){
rowIndex = record;
record = ds.getAt(rowIndex);
}else{
rowIndex = ds.indexOf(record);
}
var cs = this.getColumnData();
var p = {}, g = this.grid, cm = g.colModel, ct = ts.cell;
var innerVals = [];
var colCount = cm.getColumnCount();
var last = colCount-1
var cells = this.getRow(rowIndex).getElementsByTagName("td");
for(var i = 0; i < colCount; i++){
c = cs[i];
p.id = c.id;
p.css = i == 0 ? 'x-grid3-cell-first ' : (i == last ? 'x-grid3-cell-last ' : '');
p.attr = p.cellAttr = "";
p.value = c.renderer(r.data[c.name], p, r, rowIndex, i, ds);
p.style = c.style;
if(p.value == undefined || p.value === "") p.value = "&#160;";
Ext.fly(cells[i]).child(".x-grid3-cell-inner", true).innerHTML = p.value;
}
this.fireEvent("rowupdated", this, rowIndex, record);
}
});


This just replaces the value of the cells inner DIV.

Note, that if the extra options were used, such as setting the extra CSS in the renderer, and the new value would have produced different results, this won't work.

Also, if getRowClass was implemented to depend on cell values, and the value changed would have affected this, it won't work.

These could be fixed with some more work.

grzegorz.borkowski
12 Mar 2009, 1:07 AM
Thanks Animal, I will give it a try probably next week in my app, as now I'm working more on server-side functionality.

grzegorz.borkowski
19 Mar 2009, 12:59 AM
Ok, I've used your code: it seems to work properly in FF3, but it throws exception :
'ts' is undefined
in IE7. Obviously, IE is not able to give me any more information about it, and unfortunately I don't have any tool plugged to IE to locate where it comes from. So could you check your code under IE? Steps to reproduce: use EditorGrid with your patch applied, and change value in any cell: you should get exception. However I use some non-standard selection model, so I'm not sure if you will be able to reproduce it. If not, then perhaps you could point me to some good tool for debugging javascript in IE, so that I can find the place when it is thrown.

Animal
19 Mar 2009, 1:05 AM
Yes, "ts" isn't defined. Take a look at GridView.js, and see what it should be, and set it.

nielsjeh
29 Apr 2009, 7:50 AM
Thanks for the code Animal. I added some things to it.



Ext.override(Ext.grid.GridView, {
// private
refreshRow : function(record){
var ds = this.ds, rowIndex;
if(typeof record == 'number'){
rowIndex = record;
record = ds.getAt(rowIndex);
}else{
rowIndex = ds.indexOf(record);
}
var cs = this.getColumnData();
var ts = this.templates;
var p = {}, g = this.grid, cm = g.colModel, ct = ts.cell;
var innerVals = [];
var colCount = cm.getColumnCount();
var last = colCount-1
var cells = this.getRow(rowIndex).getElementsByTagName("td");

for(var i = 0; i < colCount; i++){
var c = cs[i];
p.id = c.id;
p.css = i == 0 ? 'x-grid3-cell-first ' : (i == last ? 'x-grid3-cell-last ' : '');

if(this.markDirty && record.dirty && typeof record.modified[c.name] !== 'undefined'){
p.css += ' x-grid3-dirty-cell';
}

p.attr = p.cellAttr = "";
p.style = c.style;

p.value = c.renderer(record.data[c.name], p, record, rowIndex, i, ds);

if(p.value == undefined || p.value === "") p.value = " ";

Ext.fly(cells[i]).replaceWith(ct.apply(p));
}

this.fireEvent("rowupdated", this, rowIndex, record);
}
});


It now replaces the complete td of the cell and adds dirty marks. Works fine for me so far, do you have any remarks?

grzegorz.borkowski
13 May 2009, 10:43 PM
I've just looked at 3.0 sources: it's a pity that the refreshRow implementation is still the same as in 2.2.1. I hoped it will be fixed in 3.0...

Animal
14 May 2009, 12:06 AM
I agree, this really needs looking at. Maybe even by Jack himself!

Theer are other questions in the forum caused by complete row replacement.

IMHO, doing Record.set() on a field should just update the associated cell, not the whole row.

mystix
14 May 2009, 12:39 AM
I agree, this really needs looking at. Maybe even by Jack himself!

Theer are other questions in the forum caused by complete row replacement.

IMHO, doing Record.set() on a field should just update the associated cell, not the whole row.

+1

i foresee performance improving too when existing DOM content innerHTML is updated as opposed to being recreated on the fly.

Animal
14 May 2009, 2:12 AM
This guy seems to be hitting this issue: http://extjs.com/forum/showthread.php?t=68418

He's dyamically changing styles, but it seems to him that they are being reset.

I think the row is being rerendered.

woozy
14 May 2009, 3:25 AM
Hi,
I've been reading this tread and that's exactly my problem. I've been trying different workarounds in the last days but I just can't get it to work. Maybe I'll give a try to the Animal's code.

woozy
14 May 2009, 7:21 AM
Thanks for the code Animal. I added some things to it.



Ext.override(Ext.grid.GridView, {
// private
refreshRow : function(record){
var ds = this.ds, rowIndex;
if(typeof record == 'number'){
rowIndex = record;
record = ds.getAt(rowIndex);
}else{
rowIndex = ds.indexOf(record);
}
var cs = this.getColumnData();
var ts = this.templates;
var p = {}, g = this.grid, cm = g.colModel, ct = ts.cell;
var innerVals = [];
var colCount = cm.getColumnCount();
var last = colCount-1
var cells = this.getRow(rowIndex).getElementsByTagName("td");

for(var i = 0; i < colCount; i++){
var c = cs[i];
p.id = c.id;
p.css = i == 0 ? 'x-grid3-cell-first ' : (i == last ? 'x-grid3-cell-last ' : '');

if(this.markDirty && record.dirty && typeof record.modified[c.name] !== 'undefined'){
p.css += ' x-grid3-dirty-cell';
}

p.attr = p.cellAttr = "";
p.style = c.style;

p.value = c.renderer(record.data[c.name], p, record, rowIndex, i, ds);

if(p.value == undefined || p.value === "") p.value = " ";

Ext.fly(cells[i]).replaceWith(ct.apply(p));
}

this.fireEvent("rowupdated", this, rowIndex, record);
}
});
It now replaces the complete td of the cell and adds dirty marks. Works fine for me so far, do you have any remarks?


This code isn't working for me. I get this exception :
uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMHTMLTableRowElement.insertBefore]" nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)" location: "JS frame :: http://192.168.16.30/extjs/ext-all-debug.js :: anonymous :: line 3871" data: no]

Am I doing anything wrong?

woozy
14 May 2009, 9:08 AM
I've noticed that this exception occurrs when I call the set method.

mcs
31 May 2009, 9:18 AM
I agree, this really needs looking at....

I'll second that; I just spent 8 hours trying to figure out why Spotlight wasn't working and the root cause was that the "expanded row" that I wanted to spotlight "during the save" went away after I set a cell in the grid to a new value.

IMO, I'd prefer that refreshRow was called in response to a succesful save since other column values are based on the server's response. so, in my case, I want the refreshRow method called after 'save' is fired not before it is fired.

Others have noted that the row body update should probably be optional since my users like saving their work ("applying" the values in my row body editor) periodically before editing another row.

In my case, I solved the problem by having my beforesave handler expand the row again and push in some new HTML bits which I can spotlight.

Your refreshRow implementation looks promising since I could leave my editor in the row body.

mystix
15 Jun 2009, 12:05 AM
@nielsjeh's implementation contains a markDirty config which is most likely a custom flag in his own application.
he did however bring up a good point about the cell's dirty flag, which again is not solved by his implementation because the cell's content is still being overwritten with a new html fragment (via the cell's Ext.Template).

the good news is, just a tiny tweak to @animal's original override is required to handle this:


Ext.override(Ext.grid.GridView, {
refreshRow : function(record) {
var ds = this.ds,
rowIndex;

if (typeof record == 'number') {
rowIndex = record;
record = ds.getAt(rowIndex);
} else {
rowIndex = ds.indexOf(record);
}

var cs = this.getColumnData(),
p = {},
g = this.grid,
cm = g.colModel,
colCount = cm.getColumnCount(),
last = colCount - 1,
cells = this.getRow(rowIndex).getElementsByTagName("td"),
i, c, mod, val;

for (i = 0; i < colCount; i++) {
c = cs[i];
mod = record.modified;

Ext.apply(p, {
id : c.id,
css : i == 0 ? 'x-grid3-cell-first ' : (i == last ? 'x-grid3-cell-last ' : ''),
attr : '',
cellAttr: '',
style : c.style
});

val = c.renderer(record.data[c.name], p, record, rowIndex, i, ds);
val = Ext.isEmpty(val) ? ' ' : val;

// update cell's dirty state
Ext.fly(cells[i])[mod && mod[c.name] !== undefined ? 'addClass' : 'removeClass']('x-grid3-dirty-cell').child('.x-grid3-cell-inner', true).innerHTML = val;
}

this.fireEvent("rowupdated", this, rowIndex, record);
}
});

note 1: this override (and @animal's override of course) also resolves the "bug with clicksToEdit:1" bug.
note 2: this override works for the 3.x branch as well.

[edit]
even with the override in place, the "clicksToEdit: 1" bug still surfaces intermittently, but only on the following browsers:
- Gecko 3 (OSX / Win)
- Opera 9.64 (OSX / Win)

sidebog7
17 Jun 2009, 7:25 AM
I'm still seeing a few weird things with this fix.

For example, if you edit cell A and then click cell B and edit that, when you click cell A to edit your previous change the cell doesn't start editing and requires a further click.

Actually, after looking at this for a while it would appear to be because my renderer is placing a span in the div cell and this is being replaced when the row is refreshed which has the same effect as the original problem.

I'm not sure this is fixable so I might move to the mousedown fix mentioned in the other thread.

mystix
17 Jun 2009, 9:03 AM
I'm still seeing a few weird things with this fix.

For example, if you edit cell A and then click cell B and edit that, when you click cell A to edit your previous change the cell doesn't start editing and requires a further click.

Actually, after looking at this for a while it would appear to be because my renderer is placing a span in the div cell and this is being replaced when the row is refreshed which has the same effect as the original problem.

I'm not sure this is fixable so I might move to the mousedown fix mentioned in the other thread.

which browser are you seeing this on? FF3 / Opera?

sidebog7
17 Jun 2009, 9:11 AM
Firefox 2.

After an hour of trying to figure out what was going on it seemed to be intermittent. When I was outputting a span with content from the renderer it would happen ~80% of the time whereas when I used a renderer with no span it seemed to still happen but only ~10% of the time.

If there is anything you want me to test I still have the test code around.

mystix
17 Jun 2009, 10:13 AM
i'm willing to wager it's a firefox / opera thing, so i wouldn't waste time investigating further
(any workaround is highly likely to employ the use of wonky deferred functions, which i'm not too fond of).

all other browsers (yes, even IE!!!) behave correctly.

Opera & Firefox 3 (as i noted in post #17 above) misbehaves just like firefox 2.

sidebog7
17 Jun 2009, 12:00 PM
Yeah, I figured. I'm happy with the mousedown fix for my particular case.

mystix
30 Jun 2009, 11:20 PM
[ moved to 3.x Bugs from 2.x Bugs ]
and a [ friendly bump ] to boot.

mystix
1 Jul 2009, 11:57 PM
[ frantic friendly bump ]

Animal
2 Jul 2009, 1:36 AM
This is surely one for 3.1? There's quite a big fix to be done.

evant
2 Jul 2009, 1:38 AM
Yeah, I think we'll defer this one, especially the row based updating and the other implications surrounding that.

mystix
2 Jul 2009, 1:54 AM
alritey then, changed the thread status to OPEN-DEF.

kai5263499
14 Jul 2009, 6:07 AM
I've been using Ext.grid.getView().refreshRow(record) in 2.2.0. Even though it isn't documented it seems to work great. Is this method not available in 3.0?

What I would like to see is a refreshCell option...

Condor
14 Jul 2009, 6:20 AM
I've been using Ext.grid.getView().refreshRow(record) in 2.2.0. Even though it isn't documented it seems to work great. Is this method not available in 3.0?

What I would like to see is a refreshCell option...

No, it's still available and it still works the same way: It removes the current row and adds a new one.

This bugreport is for the problems that removing and adding a row is causing and the suggestion was made to update the current row instead.

The problem is that updating a row introduces all kinds of other problems...

mystix
14 Jul 2009, 8:18 AM
No, it's still available and it still works the same way: It removes the current row and adds a new one.

This bugreport is for the problems that removing and adding a row is causing and the suggestion was made to update the current row instead.

The problem is that updating a row introduces all kinds of other problems...

i'm actually using the override i posted above without issues (yet).
it's being used in conjunction with a couple of overrides i have for the EditorGridPanel (pm me if you want a copy to mess with ;) ).

what issues did you experience with row updating vs replacement?

Condor
14 Jul 2009, 11:27 AM
There are several things the current override doesn't do:
- Update the row class (getRowClass could return something different).
- Update the cell class and attributes (the renderer could modify the metaData). Note: updating the attributes is going to be difficult to fix.

Also, it needs some cleanup (e.g. it should use the cellSelector instead of getElementsByTagName).

And finally, this override does indeed leave the tr, td and inner div element in tact, but it still replaces the cell content, which could still make the click event get lost (because the target was removed).

mystix
13 Aug 2009, 2:21 AM
@condor et al,

i took a second stab at this, and
- implemented cell lookup using cellSelector instead of getElementsByTagName
- managed to update the cell class and attributes (using any renderer-modified metaData) via Ext.Element#set()

here goes nothing:


Ext.override(Ext.grid.GridView, {
// private
getCells: function(row) {
return this.fly(this.getRow(row)).query(this.cellSelector);
},

// private
// returns an object containing cell attributes in a form
// which can be consumed by Ext.Element#set()
getCellAttributes: function(attrib) {
var attributes = {};

if (!(attrib && Ext.isString(attrib))) {
return attributes;
}

// note: regex assumes HTML attribute values containing double quotes
// have been converted to corresponding HTML entities
Ext.each(attrib.match(/[\w:]+=(?:'.*?'|".*?")/g), function(token) {
// each token is a valid HTML tag attribute of the form attribute="someValue"
var a = token.split('=');

// remove leading / trailing quotes
attributes[a[0]] = a[1].substring(1, a[1].length - 1);
});

return attributes;
},

// private
refreshRow : function(record) {
var ds = this.ds,
rowIndex;

if (Ext.isNumber(record)) {
rowIndex = record;
record = ds.getAt(rowIndex);
} else {
rowIndex = ds.indexOf(record);
}

if (!record || rowIndex < 0) {
return;
}

var cs = this.getColumnData(),
last = cs.length - 1,
cells = this.getCells(rowIndex),
classes = [
'x-grid3-cell-first',
'x-grid3-cell-last',
'x-grid3-dirty-cell'
],
p, cls, mod, val;

Ext.each(cs, function(cfg, idx) {
p = { // cell attributes - may be modified by cfg.renderer
id : cfg.id,
style : cfg.style,
attr : '',
cellAttr: ''
};
mod = record.modified;
cls = (mod && Ext.isDefined(mod[cfg.name]) ? ['x-grid3-dirty-cell'] : [])
.concat(idx === 0 ? ['x-grid3-cell-first'] : idx === last ? ['x-grid3-cell-last'] : []);

val = cfg.renderer(record.data[cfg.name], p, record, rowIndex, idx, ds);
val = Ext.isEmpty(val) ? ' ' : val;

Ext.fly(cells[idx])
.removeClass(classes).addClass(cls) // update cell's css classes (i.e. dirty state, first/last cell)
.child('.x-grid3-cell-inner').set(this.getCellAttributes(p.attr)) // update cell's attributes
.dom.innerHTML = val; // update cell's value
}, this);

this.fireEvent("rowupdated", this, rowIndex, record);
}
});


with that in place, i can have renderers like this:


renderer: function(v, p) {
p.attr = {
'ext:qtip': 'woot!'
};

return v + '!!!';
}

which allow cell qtips to be updated.


p.s. any ideas on the rowClass thingy you mentioned?
(i didn't look into it 'cos i don't use row classes ~o) )

p.s.s. cell content replacement is still an outstanding issue.
i don't foresee this being a major issue though. thoughts?

[edit]
that code broke initial rendering of grid cells with custom attr metaData.
also realised i shouldn't be fiddling with cell metaData. updated the code above.

fangzhouxing
28 Aug 2009, 5:06 PM
hi, mystix, I used your code in my project of Ext 2.3.0, is it ok? (I removed Ext.isNumber and Ext.isDefined).

mystix
28 Aug 2009, 9:07 PM
hi, mystix, I used your code in my project of Ext 2.3.0, is it ok? (I removed Ext.isNumber and Ext.isDefined).

be my guest :)

please report any issues you encounter with that
-- i'm using it in production too and it'd be good to see how it stands up in general.

mankz
30 Aug 2009, 12:17 AM
@Mystix: Would it be possible to tweak your code to allow a refreshCell method which would refresh a single cell?

mystix
30 Aug 2009, 1:37 AM
@Mystix: Would it be possible to tweak your code to allow a refreshCell method which would refresh a single cell?

i don't see why it wouldn't be possible.
care to take a stab? ;)

mankz
30 Aug 2009, 1:39 AM
I think I will, I think it would make a difference in my scenario where a refreshRow is very expensive involving lots of rendering logic. Will report back during the day I hope :)

mystix
30 Aug 2009, 1:54 AM
I think I will, I think it would make a difference in my scenario where a refreshRow is very expensive involving lots of rendering logic. Will report back during the day I hope :)

since it's a sunday, plus i'm lazy, i'll sit here and wait in eager anticipation for your good news :)

alco
5 Nov 2009, 10:16 PM
be my guest :)

please report any issues you encounter with that
-- i'm using it in production too and it'd be good to see how it stands up in general.

i'm not exactly sure, but would it be safe to add a 'beforerowupdated' event to give chance to cleanup RowExpander, etc. plugins?

mystix
22 Mar 2010, 8:32 AM
i'm not exactly sure, but would it be safe to add a 'beforerowupdated' event to give chance to cleanup RowExpander, etc. plugins?

i don't see why not (sorry, just saw this message).