Hi Pavel, Thanks a lot for review.
Best regards! - Jonathan 2012/4/19 Pavel Porvatov <pavel.porva...@oracle.com> > Hi Jonathan, > > Hi Pavel, >> >> I found Util.invokeOnEDT in awt repository, and have updated the test >> case, could you please take another look? >> http://cr.openjdk.java.net/~**luchsh/7055065_2/<http://cr.openjdk.java.net/%7Eluchsh/7055065_2/> >> > The fix looks good. I approve it. > > >> It indeed confused me when found the change in awt repository, will it be >> merged to swing repository or is it a special change for testing >> infrastructure so went to awt repository? >> > Swing repository is a legacy one and it's not used. I hope it will be > removed soon to avoid such confusion. > > Regards, Pavel > > >> Thanks and best regards! >> - Jonathan >> >> On 04/19/2012 11:24 AM, Jonathan Lu wrote: >> >>> 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/<http://cr.openjdk.java.net/%7Eluchsh/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/<http://cr.openjdk.java.net/%7Eluchsh/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.**setPreferredScrollableViewport**Size(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 >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
