PDA

View Full Version : [OPEN TOUCH-138] Serious flaw with List Selection (revised)



jep
1 Jun 2011, 2:13 PM
This is a simplified version of this bug (http://www.sencha.com/forum/showthread.php?111979-FIXED-414-0.96-Serious-Flaw-with-List-Selection&highlight=grouped+items+select) that was marked as FIXED but never was actually fixed. I'm thinking the previous one was possibly too complex because it seemed that no one ever actually verified it was fixed.

Here's the code:



<html>
<head>
<title>test</title>

<link rel="stylesheet" href="../sencha/resources/css/sencha-touch.css" type="text/css">
<script type="text/javascript" src="../sencha/sencha-touch-debug.js"></script>

<script type="text/javascript">

Ext.regModel('Story', {
fields: ['grouper', 'title']
});

var store = new Ext.data.Store({
model: 'Story',
groupField:'grouper',
data:[
{grouper:'Group 1', title:'Group 1 Item A'},
{grouper:'Group 2', title:'Group 2 Item A'},
{grouper:'Group 1', title:'Group 1 Item B'}
]
});
var list = new Ext.List({
itemTpl: '<div class="story"><strong>{title}</strong></div>',
store: store,
grouped: true,
listeners:{
itemtap: function(view, index, item, e) {
var record = view.store.getAt(index);
alert('Selected ' + record.get('title'));
}
}
});


Ext.setup({
onReady: function() {
var homeScreen = new Ext.Panel({
layout: 'fit',
fullscreen: true,
style: 'border: 5px solid #666666;background-color:#FFFFFF',
items: [list]
});
}
});

</script>

</head>
<body></body>
</html>


To reproduce this bug, click on the various items. When I click on "Group 1 Item B", the popup tells me I clicked on "Group 2 Item A". When I click on "Group 2 Item A", it says "Group 1 Item B".

It just really seems to have major problems when the data isn't in the exact same order as it will be after grouping.

This occurs in:
Sencha Touch 1.1.0
iPhone iOS 4.0
Safari XP 5.0.3
Chrome XP 11.0.696.71

mike.estes
2 Jun 2011, 8:17 AM
I was able to reproduce this issue last night. I'm working on filing a bug report for this today, will update this thread and the other related thread accordingly.

jep
2 Jun 2011, 8:22 AM
Thank you for your response. I note that if you apply a sorter to force the data to be sorted in the same way it is displayed, you don't get the problem. However, this doesn't work as well when sorting is independent of grouping. For example, I want to sort a list of hotel rates by price but then group them by region:

Original data:

$299 - Hotel 1 - Downtown
$399 - Hotel 2 - Uptown
$499 - Hotel 3 - Downtown
$599 - Hotel 4 - Uptown

Displayed grouped data:

Downtown
- $299 - Hotel 1
- $499 - Hotel 3
Uptown
- $399 - Hotel 2
- $599 - Hotel 4

jep
2 Jun 2011, 8:24 AM
Argh, sorry. Forum still has the bug where clicking on "Go Advanced" actually posts my message. Was just going to say I was trying to head off the answer of "just apply a sorter" before it happens.

jep
2 Jun 2011, 1:12 PM
The more I look at this, the more I wonder if it's just a definition/documentation problem, maybe additionally one that could use another function to help translate from the "display" index to the "store" index. I dug into it quite a bit, and the problem is that the tapitem event handler gets an index param that is the index of the tapped node in list.all.elements (via list.findTargetByEvent(e)). This index is just not going to always be the same when grouping is involved. What I'm thinking is maybe if theire was a displayIndexToRecord or similar function.

Here's what I've come up with as a workaround:

(Edit note: I had to add a bit more than just displayIndexToRecord like I originally thought. See next post.)



Ext.namespace("Ext.jep");

Ext.jep.List = Ext.extend(Ext.List, {
displayIndexToRecordIndex: function (targetIndex) {
if (this.grouped) {
var groups = this.getStore().getGroups();

for (var g = 0; g < groups.length; g++) {
var group = groups[g].children;

if (targetIndex < group.length)
return this.getStore().indexOf(group[targetIndex]);

targetIndex -= group.length;
}
}
else
return targetIndex;
},

recordIndexToDisplayIndex: function (targetIndex) {
if (this.grouped) {
var rec = this.getStore().getAt(targetIndex);

var groups = this.getStore().getGroups();
var currentIndex = 0;

for (var g = 0; g < groups.length; g++) {
var group = groups[g].children;

for (var i = 0; i < group.length; i++)
if (group[i] == rec)
return currentIndex;
else
currentIndex++;
}
}
else
return targetIndex;
},

getNode: function (nodeInfo) {
if (Ext.isString(nodeInfo)) {
return document.getElementById(nodeInfo);
} else if (Ext.isNumber(nodeInfo)) {
return this.all.elements[nodeInfo];
} else if (nodeInfo instanceof Ext.data.Model) {
var idx = this.recordIndexToDisplayIndex(this.store.indexOf(nodeInfo));
return this.all.elements[idx];
}
return nodeInfo;
},

getRecord: function(node){
return this.store.getAt(this.displayIndexToRecordIndex(node.viewIndex));
}
});
Ext.reg('jeplist', Ext.jep.List);


And here's the example posted earlier with the workaround integrated:


<html>
<head>
<title>test</title>

<link rel="stylesheet" href="../sencha/resources/css/sencha-touch.css" type="text/css">
<script type="text/javascript" src="../sencha/sencha-touch-debug-w-comments.js"></script>

<script type="text/javascript">
Ext.namespace("Ext.jep");

Ext.jep.List = Ext.extend(Ext.List, {
displayIndexToRecordIndex: function (targetIndex) {
if (this.grouped) {
var groups = this.getStore().getGroups();

for (var g = 0; g < groups.length; g++) {
var group = groups[g].children;

if (targetIndex < group.length)
return this.getStore().indexOf(group[targetIndex]);

targetIndex -= group.length;
}
}
else
return targetIndex;
},

recordIndexToDisplayIndex: function (targetIndex) {
if (this.grouped) {
var rec = this.getStore().getAt(targetIndex);

var groups = this.getStore().getGroups();
var currentIndex = 0;

for (var g = 0; g < groups.length; g++) {
var group = groups[g].children;

for (var i = 0; i < group.length; i++)
if (group[i] == rec)
return currentIndex;
else
currentIndex++;
}
}
else
return targetIndex;
},

getNode: function (nodeInfo) {
if (Ext.isString(nodeInfo)) {
return document.getElementById(nodeInfo);
} else if (Ext.isNumber(nodeInfo)) {
return this.all.elements[nodeInfo];
} else if (nodeInfo instanceof Ext.data.Model) {
var idx = this.recordIndexToDisplayIndex(this.store.indexOf(nodeInfo));
return this.all.elements[idx];
}
return nodeInfo;
},

getRecord: function(node){
return this.store.getAt(this.displayIndexToRecordIndex(node.viewIndex));
}
});
Ext.reg('jeplist', Ext.jep.List);

Ext.regModel('Story', {
fields: ['grouper', 'title']
});

var store = new Ext.data.Store({
model: 'Story',
groupField:'grouper',
data:[
{grouper:'Group 1', title:'Group 1 Item A'},
{grouper:'Group 2', title:'Group 2 Item A'},
{grouper:'Group 1', title:'Group 1 Item B'}
]
});
var list = new Ext.jep.List({
itemTpl: '<div class="story"><strong>{title}</strong></div>',
store: store,
grouped: true,
listeners:{
itemtap: function(view, index, item, e) {
var record = view.store.getAt(list.displayIndexToRecordIndex(index));
alert('itemTap: ' + record.get('title'));
},
selectionchange: function(selectionModel, records) {
if (records.length)
alert('selectionchange: ' + records[0].get('title'));
}
}
});


Ext.setup({
onReady: function() {
var homeScreen = new Ext.Panel({
layout: 'fit',
fullscreen: true,
style: 'border: 5px solid #666666;background-color:#FFFFFF',
items:[list]
});
}
});

</script>

</head>
<body></body>
</html>


Edit: Fixed the code above so it works with selectionchange and other events.

jep
2 Jun 2011, 3:00 PM
Of course, it's never as easy as you think. I found my original idea of just using displayIndexToRecordIndex in the itemtap handler wasn't sufficient to fix the selectionchange event, which doesn't pass indexes but passes actual records (and the wrong ones, in this case).

So I had to do a bit more thorough work to root out the problem.

Note, though, that I still have to use displayIndexToRecordIndex in the itemtap event handler. I could have went further and rewrote list.indexOf, which would cause the index sent to itemtap to be a recordIndex rather than an index into list.all.elements. But I'm not sure exactly what other stuff might depend on list.indexOf returning the ACTUAL index into list.all.elements.

mike.estes
2 Jun 2011, 4:23 PM
Yeah, I went through that same process as well. I have an idea for a work around that I am trying to flesh out a bit. This is useful info for the bug report so I won't be filing that until tomorrow once I am done with my work around. I'll update you tomorrow with some more info.

jep
9 Jun 2011, 2:30 PM
Related:
http://www.sencha.com/forum/showthread.php?136522-Here-s-a-list-that-can-be-grouped-ungrouped-and-fixes-some-other-grouping-bugs&p=612365#post612365

samsonasu
19 Jul 2011, 12:24 PM
Definitely a huge bug. Thanks for the patch jeb.

I realize this is complicated but I think a reasonable workaround would be to have the itemtap listener receive the actual record from the store instead of the index (which clearly isn't reliable right now).

elmasse
19 Jul 2011, 12:35 PM
Related: http://www.sencha.com/forum/showthread.php?138547-viewIndex-wrong-value-when-adding-item-on-grouped-list

edspencer
19 Jul 2011, 11:45 PM
Ah - this is part of the backlog for 2.0 at the moment. The problem is in how we assign ids to the generated DOM elements - we don't use a properly unique key for this so grouping throws it off. I suspect we could backport the solution to 1.x though (you'll like 2.0 a lot more)