Mathias, what all should tests cover in this case?
Here's one possibility -- it tests that changing a value works and
saves the state, then tests that deleting a row properly adjusts the
saved row state.
/*
* Test method for
* 'org.apache.myfaces.component.html.ext.HtmlDataTable
preserveRowStates=true'
*/
public void testPreserveRowStatesTrue()
{
String OUTER_UI_DATA_ID = "outerUIData";
String INNER_UI_DATA_ID = "innerUIData";
HtmlDataTable outerUIData = new HtmlDataTable();
outerUIData.setId(OUTER_UI_DATA_ID);
outerUIData.setPreserveRowStates(true);
List li = new ArrayList();
li.add(new TestData2("outer1", "outer1"));
li.add(new TestData2("outer2", "outer2"));
li.add(new TestData2("outer3", "outer3"));
li.add(new TestData2("outer4", "outer4"));
li.add(new TestData2("outer5", "outer5"));
outerUIData.setValue(new ListDataModel(li));
outerUIData.setVar("testData2");
UIColumn column = new UIColumn();
outerUIData.getChildren().add(column);
UIInput input = new UIInput();
input.setId("input");
input.setValueBinding("value",
createValueBinding("#{testData2.description}"));
column.getChildren().add(input);
UIColumn column2 = new UIColumn();
outerUIData.getChildren().add(column2);
HtmlDataTable innerUIData = new HtmlDataTable();
innerUIData.setId(INNER_UI_DATA_ID);
innerUIData.setPreserveRowStates(true);
innerUIData.setValueBinding("value",
createValueBinding("#{testData2.testDataList}"));
innerUIData.setVar("testData");
UIColumn innerInputColumn = new UIColumn();
innerUIData.getChildren().add(innerInputColumn);
UIInput innerInput = new UIInput();
innerInput.setId("innerInput");
innerInput.setValueBinding("value",
createValueBinding("#{testData.description}"));
innerInputColumn.getChildren().add(innerInput);
column.getChildren().add(innerUIData);
facesContext.getViewRoot().getChildren().add(outerUIData);
UIComponent comp = outerUIData.findComponent(":" +
OUTER_UI_DATA_ID + ":1:" + INNER_UI_DATA_ID + ":1:innerInput");
assertTrue(comp instanceof UIInput);
UIInput uiInput = (UIInput) comp;
String originalValue = (String)uiInput.getValue();
String newValue = "new value";
assertEquals(originalValue, "outer2-2");
outerUIData.setRowIndex(1);
innerUIData.setRowIndex(1);
UIColumn outerColumn1 = ((UIColumn)innerUIData.getChildren().get(0));
UIInput rawInput = ((UIInput)outerColumn1.getChildren().get(0));
assertEquals(rawInput.getValue(), originalValue);
rawInput.setValue(newValue);
innerUIData.setRowIndex(-1);
outerUIData.setRowIndex(-1);
comp = outerUIData.findComponent(":" + OUTER_UI_DATA_ID +
":1:" + INNER_UI_DATA_ID + ":1:innerInput");
assertTrue(comp instanceof UIInput);
assertEquals(((UIInput)comp).getValue(), newValue);
outerUIData.setRowIndex(1);
innerUIData.setRowIndex(1);
TestData2 testData2 = (TestData2)outerUIData.getRowData();
testData2.getTestDataList().remove(innerUIData.getRowData());
innerUIData.setRowIndex(-1);
outerUIData.setRowIndex(-1);
comp = outerUIData.findComponent(":" + OUTER_UI_DATA_ID +
":1:" + INNER_UI_DATA_ID + ":1:innerInput");
assertTrue(comp instanceof UIInput);
assertEquals("outer2-3", ((UIInput)comp).getValue());
}
On 4/12/07, Mathias Brökelmann <[EMAIL PROTECTED]> wrote:
Mike please create a unit test for this (including one for a nested
table). It will help us tracking this issue if other things are
changing in this area.
2007/4/12, Mike Kienenberger <[EMAIL PROTECTED]>:
> Ok. After making these two changes (use rowData as key, record last
> rowData object and only save input component state if the rowData
> object hasn't changed on setRowIndex()), everything seems to be
> working. I'm going to test nested datatables using the
> countryTableForm.jsp page, and that should be it.
>
> The only question is whether I should make the new way of doing things
> the default behavior for preserveRowStates or if I need to create a
> preserveRowStatesMode to determine whether to use the old or new
> behavior.
>
> On 4/12/07, Mike Kienenberger <[EMAIL PROTECTED]> wrote:
> > I've given this more thought overnight. I don't think _rowStates is
> > saved between requests. It only lives for the duration of one
> > Lifecycle iteration. This appears to be true for client-side state
> > saving. I'll need to double-check that it's also true for server-side
> > state saving as I'm pretty ignorant in that area. Thus, there's no
> > reason why we shouldn't be able to use the row data itself as the key
> > instead of a converter String based on the row data. In both cases,
> > we'd still have the issue with the same object appearing in the data
> > model, but again, I'm not sure what the use case would be.
> >
> > Also, the solution for the issue above is probably handled with the
> > following logic -- record the last data row object at the end of
> > setRowIndex(). If the last row object doesn't match the current row
> > object at the start of setRowIndex(), then don't try to save the row
> > state. We'd assume that the last row object is null at the start of
> > the lifecycle (I think rowIndex starts at -1 at the start of the
> > lifecycle as well, so this should be safe).
> >
> > One thing we might want to do is to have an attribute that specifies
> > preserve row state behavior. One option for the methodology above
> > which is save for sort/add/delete, but not safe for multiple copies of
> > the same rowData. One option for clientid (or maybe row number)
> > which is safe for multiple copies but unsafe for changes to the
> > dataModel.
> >
> > On 4/12/07, Martin Marinschek <[EMAIL PROTECTED]> wrote:
> > > the question is if we shouldn't dig deeper - shouldn't for all
> > > row-handling your special converter be used instead of the row-index?
> > > Trinidad has something similar (row-key).
> > >
> > > regards,
> > >
> > > Martin
> > >
> > > On 4/12/07, Mike Kienenberger <[EMAIL PROTECTED]> wrote:
> > > > Ok. Here's why. The data for row 2 is copied to the input
> > > > components, row data 2 is deleted, then the data from the input
> > > > components (still for the original row 2) is copied back to row 2.
> > > >
> > > > ====================
> > > > before invokeAction
> > > > setRowIndex(2)
> > > > copy row 2 data to input components
> > > > 22:08:32.578 INFO [SocketListener0-1]
> > > >
org.apache.myfaces.component.html.ext.HtmlDataTableHack.setRowIndex(HtmlDataTableHack.java:290)
> > > > >35> Fetching rowState for key=Item2, _rowIndex=1, rowData=Item #2,
> > > > got [[Ljava.lang.Object;@650646]
> > > >
> > > > delete row data 2.
> > > >
> > > > setRowIndex(-1)
> > > > copy input component values to row 2
> > > > 22:08:32.578 INFO [SocketListener0-1]
> > > >
org.apache.myfaces.component.html.ext.HtmlDataTableHack.setRowIndex(HtmlDataTableHack.java:245)
> > > > >35> Storing rowState for key=Item3, _rowIndex=1, rowData=Item #3,
> > > > value=[[Ljava.lang.Object;@1a32ea4]
> > > > ==================================
> > > >
> > > >
> > > > I'm not sure what a good fix for this would be. Perhaps detecting
> > > > that row data has changed in the invoke action phase? Perhaps caching
> > > > the original rows -> rowData associations before invoke action and
> > > > ignoring changes when the row data no longer matches the same row
> > > > instead of saving the components, or always using the cached values
> > > > during this phase?
> > > >
> > > >
> > > >
> > > > On 4/11/07, Mike Kienenberger <[EMAIL PROTECTED]> wrote:
> > > > > Well, this is kinda odd.
> > > > >
> > > > > I've implemented the converter version, and it *almost* works.
> > > > >
> > > > > Without the converter, using the standard clientId key,
> > > > >
> > > > > public String getAsString(FacesContext context,
UIComponent
> > > > > component, Object value)
> > > > > {
> > > > > UIData uiData = (UIData)component;
> > > > > return uiData.getClientId(context);
> > > > > }
> > > > >
> > > > > If you have
> > > > >
> > > > > row 1 - 1111
> > > > > row 2 - 2222
> > > > > row 3 - 3333
> > > > > row 4 - 4444
> > > > > row 5 - 5555
> > > > >
> > > > > and you delete row 2, you get:
> > > > >
> > > > > row 1 - 1111
> > > > > row 3 - 2222
> > > > > row 4 - 3333
> > > > > row 5 - 4444
> > > > >
> > > > > Now if I throw in a converter that maps to an object:
> > > > >
> > > > > public String getAsString(FacesContext context, UIComponent
> > > > > component, Object value)
> > > > > {
> > > > > UIData uiData = (UIData)component;
> > > > > if (uiData.isRowAvailable())
> > > > > {
> > > > > Item item = (Item)uiData.getRowData();
> > > > > return "Item" + String.valueOf(item.getId());
> > > > > }
> > > > > return uiData.getClientId(context);
> > > > > }
> > > > >
> > > > > If you have
> > > > >
> > > > > row 1 - 1111
> > > > > row 2 - 2222
> > > > > row 3 - 3333
> > > > > row 4 - 4444
> > > > > row 5 - 5555
> > > > >
> > > > > and you delete row 2, you get:
> > > > >
> > > > > row 1 - 1111
> > > > > row 3 - 2222
> > > > > row 4 - 4444
> > > > > row 5 - 5555
> > > > >
> > > > > Weird. Everything is correct except for row 3 (which picked up the
> > > > > values for row 2), which is a vast improvement over the original, but
> > > > > still not quite right.
> > > > >
> > > > >
> > > > > On 4/11/07, Mike Kienenberger <[EMAIL PROTECTED]> wrote:
> > > > > > I don't think this will affect the nested datatables since we're not
> > > > > > changing the value, only the key, for each row-state map entry.
> > > > > > However, I might be misunderstanding something.
> > > > > >
> > > > > > A starting point could be
> > > > > >
> > > > > > private Map _rowStates = new WeakHashMap();
> > > > > >
> > > > > > Then, we'd change this:
> > > > > >
> > > > > > _rowStates.put(getClientId(facesContext),
> > > > > >
saveDescendantComponentStates(getChildren()
> > > > > > .iterator(), false));
> > > > > >
> > > > > > to
> > > > > >
> > > > > > _rowStates.put(dataModel.getRowData(),
> > > > > >
saveDescendantComponentStates(getChildren()
> > > > > > .iterator(), false));
> > > > > >
> > > > > > Note, for describing this, I'm ignoring how we'd handle
isRowAvailable=false.
> > > > > > I'm guessing this would require that rowData be serializable.
> > > > > >
> > > > > > A better implementation might be:
> > > > > >
> > > > > > Converter converter = .... [either assigned explicitly
or
> > > > > > fetched by RowData type]
> > > > > >
> > > > > >
_rowStates.put(converter.getAsString(dataModel.getRowData()),
> > > > > >
saveDescendantComponentStates(getChildren()
> > > > > > .iterator(), false));
> > > > > >
> > > > > > Now we're no longer storing a weak reference to the model object
> > > > > > (good), but we'll now have to be responsible for keeping the
> > > > > > _rowStates map cleaned up (bad). Now we won't have serialization
> > > > > > issues (good).
> > > > > >
> > > > > > One potential snag might be if the backing data model contains
> > > > > > identical objects. I can't think of a practical use case for this
> > > > > > (inputs on multiple rows for the same object), but that doesn't mean
> > > > > > that there isn't one.
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 4/11/07, Martin Marinschek <[EMAIL PROTECTED]> wrote:
> > > > > > > Your ideas sound good, one thing that I remember was discussed
while
> > > > > > > implementing preserveRowStates - there were some problems with
nested
> > > > > > > data-tables - you'd need to work around those problems
specifically.
> > > > > > >
> > > > > > > What would be your idea of a key based on the row-data?
> > > > > > >
> > > > > > > regards,
> > > > > > >
> > > > > > > Martin
> > > > > > >
> > > > > > > On 4/11/07, Mike Kienenberger <[EMAIL PROTECTED]> wrote:
> > > > > > > > I'm looking into the behavior of preserveRowStates in order to
try to
> > > > > > > > fix the issues with deleting a row when preserveRowStates=true.
> > > > > > > >
> > > > > > > > One of the things I notice is that the key to the row state Map
is the
> > > > > > > > clientid of the dataTable, which of course varies based on the
row
> > > > > > > > index. Is there some reason why the key isn't the row index?
> > > > > > > >
> > > > > > > > What about the possibility of storing the row data in the map
using a
> > > > > > > > key based on the row data itself? That way, changing the row
model
> > > > > > > > (adding/deleting rows) would automatically pick the right row
state?
> > > > > > > > The only issues are that we might be keeping the row state for
rows
> > > > > > > > that no longer in the model, and we might be holding onto a
reference
> > > > > > > > to an object that should be garbage collected (I've never
delved into
> > > > > > > > this, but my understanding is that there are ways around
referencing
> > > > > > > > things that should perhaps be garbage-collected).
> > > > > > > >
> > > > > > > > However, if it works, it would automaticallly solve all of the
row
> > > > > > > > issues transparently to the end-user. Sorting, inserting,
deleting
> > > > > > > > rows would work transparently.
> > > > > > > >
> > > > > > > > We might also want to put in a preserveRowStatesConverter so we
save a
> > > > > > > > light-weight key to the row data rather than the row data
itself like
> > > > > > > > f:selectItems does.
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > >
> > > > > > > http://www.irian.at
> > > > > > >
> > > > > > > Your JSF powerhouse -
> > > > > > > JSF Consulting, Development and
> > > > > > > Courses in English and German
> > > > > > >
> > > > > > > Professional Support for Apache MyFaces
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > >
> > > http://www.irian.at
> > >
> > > Your JSF powerhouse -
> > > JSF Consulting, Development and
> > > Courses in English and German
> > >
> > > Professional Support for Apache MyFaces
> > >
> >
>
--
Mathias
