<Swing Dev> Review rquest for 7055065, NPE from JTable when quit editing with empty value in number culumns

[Jonathan Lu]

Hi Pavel,

After a simple grep, I did not see any invokeOnEDT method from swing or 
2d repository, has it been committed yet?

Thanks and best regards!
- Jonathan

On 04/16/2012 07:51 PM, Pavel Porvatov wrote:
> Hi Jonathan,
>> Hi Pavel,
>>
>> Thanks for reviewing, here's the webrev patch and automatic test. 
>> Could you please help to take another look?
>> http://cr.openjdk.java.net/~luchsh/7055065/
> The fix looks good. Could you please fix few minor changes:
>
> 1. Don't use full class names like javax.swing.SwingUtilities when 
> possible. I didn't find such rule in our Code Conventions but we 
> follow this rule.
>
> 2. Swap two lines please
>   frame.setVisible(true);
>   frame.setLocation(0, 0);
> That's not critical for the test but we shouldn't provide bad examples
>
> 3. I've recently introduced the Util#invokeOnEDT method. It can 
> simplify your test and the getHeaderClickPoint and getCellClickPoint 
> methods can be removed.
>
> Regards, Pavel
>>
>> Thanks & regards!
>> - Jonathan
>>
>> On 04/13/2012 05:59 PM, Pavel Porvatov wrote:
>>> Hi Jonathan,
>>>
>>> The fix looks good. Could you please write an automatic test, put it 
>>> into an appropriate place of repository and make a webrev with fix 
>>> and test?
>>>
>>> Regards, Pavel
>>>> Hello swing-dev,
>>>>
>>>> I've got a patch for bug 7055065, can anybody please help to take a 
>>>> look?
>>>> http://cr.openjdk.java.net/~luchsh/7055065/
>>>>
>>>> And also a simple test case for this issue here.
>>>>
>>>> /*
>>>>  * Copyright (c) 2012 Oracle and/or its affiliates. All rights 
>>>> reserved.
>>>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>>  *
>>>>  * This code is free software; you can redistribute it and/or 
>>>> modify it
>>>>  * under the terms of the GNU General Public License version 2 
>>>> only, as
>>>>  * published by the Free Software Foundation.
>>>>  *
>>>>  * This code is distributed in the hope that it will be useful, but 
>>>> WITHOUT
>>>>  * ANY WARRANTY; without even the implied warranty of 
>>>> MERCHANTABILITY or
>>>>  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
>>>> License
>>>>  * version 2 for more details (a copy is included in the LICENSE 
>>>> file that
>>>>  * accompanied this code).
>>>>  *
>>>>  * You should have received a copy of the GNU General Public 
>>>> License version
>>>>  * 2 along with this work; if not, write to the Free Software 
>>>> Foundation,
>>>>  * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>>>  *
>>>>  * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 
>>>> 94065 USA
>>>>  * or visit www.oracle.com if you need additional information or 
>>>> have any
>>>>  * questions.
>>>>  */
>>>>
>>>> /*
>>>>  * Portions Copyright (c) 2012 IBM Corporation
>>>>  */
>>>>
>>>> import javax.swing.JFrame;
>>>> import javax.swing.JPanel;
>>>> import javax.swing.JScrollPane;
>>>> import javax.swing.JTable;
>>>> import javax.swing.table.AbstractTableModel;
>>>> import javax.swing.table.TableModel;
>>>> import javax.swing.table.TableRowSorter;
>>>> import java.awt.Dimension;
>>>> import java.awt.GridLayout;
>>>>
>>>> public class TableDemo extends JPanel {
>>>>
>>>>     public TableDemo() {
>>>>         super(new GridLayout(1, 0));
>>>>
>>>>         final String[] columnNames = { "String", "Number" };
>>>>         final Object[][] data = { { "aaaa", new Integer(5) },
>>>>                 { "bbbb", new Integer(3) }, { "cccc", new 
>>>> Integer(2) },
>>>>                 { "dddd", new Integer(20) }, { "eeee", new 
>>>> Integer(10) } };
>>>>         final JTable table = new JTable(data, columnNames);
>>>>
>>>>         table.setPreferredScrollableViewportSize(new Dimension(500, 
>>>> 400));
>>>>         table.setFillsViewportHeight(true);
>>>>         TableModel dataModel = new AbstractTableModel() {
>>>>
>>>>             public int getColumnCount() {
>>>>                 return columnNames.length;
>>>>             }
>>>>             public int getRowCount() {
>>>>                 return data.length;
>>>>             }
>>>>             public Object getValueAt(int row, int col) {
>>>>                 return data[row][col];
>>>>             }
>>>>             public String getColumnName(int column) {
>>>>                 return columnNames[column];
>>>>             }
>>>>             public Class<?> getColumnClass(int c) {
>>>>                 return getValueAt(0, c).getClass();
>>>>             }
>>>>             public boolean isCellEditable(int row, int col) {
>>>>                 return col != 5;
>>>>             }
>>>>             public void setValueAt(Object aValue, int row, int 
>>>> column) {
>>>>                 data[row][column] = aValue;
>>>>             }
>>>>         };
>>>>         table.setModel(dataModel);
>>>>         TableRowSorter<TableModel> sorter = new 
>>>> TableRowSorter<TableModel>(
>>>>                 dataModel);
>>>>         table.setRowSorter(sorter);
>>>>
>>>>         JScrollPane scrollPane = new JScrollPane(table);
>>>>         add(scrollPane);
>>>>     }
>>>>
>>>>     public static void main(String[] args) throws Exception {
>>>>         javax.swing.SwingUtilities.invokeAndWait(new Runnable() {
>>>>             public void run() {
>>>>                 JFrame frame = new JFrame("SimpleTableDemo");
>>>>                 frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
>>>>
>>>>                 TableDemo newContentPane = new TableDemo();
>>>>                 newContentPane.setOpaque(true);
>>>>                 frame.setContentPane(newContentPane);
>>>>
>>>>                 frame.pack();
>>>>                 frame.setVisible(true);
>>>>             }
>>>>         });
>>>>     }
>>>> }
>>>>
>>>> To reproduce the problem,
>>>> please click on one cell from the "Number" column, delete all the 
>>>> content but do not press enter to quit editing status, then click 
>>>> the column title of "Number" column to sort, NPE will be thrown.
>>>>
>>>> The cause of this problem is when trying to quit editing with empty 
>>>> content of a cell  and also try to accept the partially edited 
>>>> value using stopCellEditing(), following two actions will be taken.
>>>> 1, the cellEditor of current table will be set to null.
>>>> 2, the value type of this column does not have a constructor to 
>>>> accept "Object[]" parameter, false will be returned.
>>>> Then swing will try to cancel edition without accepting the 
>>>> partially edited value using cancelCellEditing() which will get 
>>>> null for the value of cellEditor thus lead to this NPE.
>>>>
>>>> The patch is trying to return earlier before the type compatibility 
>>>> check of partially edited values when empty cell values encountered.
>>>>
>>>> Cheers!
>>>> - Jonathan
>>>>
>>>
>>
>