myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mathias Brökelmann" <mbroekelm...@googlemail.com>
Subject Re: dataTable row state saving / preserveRowStates / keying row states by row data
Date Fri, 13 Apr 2007 10:40:46 GMT
looks good.

Big thanks for your work Mike!

It would be great if we could have a unit test for each issue along
with the fix for it in myfaces in the future.

While working on myfaces 1.2 I found a lot of code which is really
hard to understand and sometimes impossible to find the reason why it
is coded in a specific way. Although the TCK covers a lot of issues
but running the tck is not quite useful since it takes a lot of
resources and time and it is not integrated in the maven test build.

2007/4/13, Mike Kienenberger <mkienenb@gmail.com>:
> 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 <mbroekelmann@googlemail.com> 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 <mkienenb@gmail.com>:
> > > 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 <mkienenb@gmail.com> 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 <martin.marinschek@gmail.com> 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 <mkienenb@gmail.com> 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 <mkienenb@gmail.com> 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 <mkienenb@gmail.com>
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 <martin.marinschek@gmail.com>
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 <mkienenb@gmail.com>
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
> >
>


-- 
Mathias

Mime
View raw message