<Swing Dev> [9] Review request for 8036983 - JAB:Multiselection Ctrl+CursorUp/Down and ActivateDescenderPropertyChanged event

[Alexander Scherbatiy]

On 4/11/2014 10:30 PM, Vivi An wrote:
> Hello Alex,
>
> On 4/11/2014 6:21 AM, Alexander Scherbatiy wrote:
>>
>> src/share/classes/javax/swing/tree/DefaultTreeSelectionModel.java
>> 208      * The lead path is set to the last unique path.
>>
>> This comment contradicts with the:
>> http://docs.oracle.com/javase/7/docs/api/javax/swing/tree/TreeSelectionModel.html 
>>
>>    The lead TreePath is the last path that was added (or set).
>> and with:
>> http://docs.oracle.com/javase/7/docs/api/javax/swing/tree/DefaultTreeSelectionModel.html#leadPath 
>>
>>   protected TreePath leadPath
>>   Last path that was added.
> Hmm, the preceding comment of 208 was the old one,  did not feel like 
> conflict with the doc. I added "selected" to try to make it clearer. 

    I see. It seems that the "last unique path" phrase is ambiguous.

   It has been added after the fix JDK-6210002 Undefined behavior of 
DefaultTreeSelectionModel.getSelectionPath() after setSelectionPaths.
   Before the fix the javadoc was:
http://docs.oracle.com/javase/6/docs/api/javax/swing/tree/DefaultTreeSelectionModel.html#setSelectionPaths%28javax.swing.tree.TreePath[]%29
     "The lead path is set to the last path in pPaths."

   It was found that the setSelectionPaths method works differently for 
the duplicated paths.
   For example:
   ----------------
   model.setSelectionPaths(new TreePath[] {path1, path2, path1});
    // lead path is path2 (last set path is path1)
   ----------------

   It was decided not to change the setSelectionPaths method algorithm 
to not break the already written applications
   and just update the specification from "The lead path is set to the 
last path in pPaths." to "The lead path is set to the last unique path."
   So "unique path" refers to paths in the current setSelectionPaths 
arguments.


   You fix changes the current setSelectionPaths behavior from:
   ----------------
   model.setSelectionPaths(new TreePath[] {path1, path2, path3});
    // lead path is path3 (last set path is path3)
   model.setSelectionPaths(new TreePath[] {path1, path2, newpath, path3});
    // lead path is path3 (last set path is path3)
   ----------------
  to
   ----------------
   model.setSelectionPaths(new TreePath[] {path1, path2, path3});
    // lead path is path3 (last set path is path3)
   model.setSelectionPaths(new TreePath[] {path1, path2, newpath, path3});
    // lead path is newpath (last set path is path3)
   ----------------

  This also can break existed applications that relies on the 
setSelectionPaths() method.

  I think that the DefaultTreeSelectionModel.setSelectionPaths() 
documentation should be updated to avoid ambiguous meaning.

  You fix should use the current 
DefaultTreeSelectionModel.setSelectionPaths() method that sets lead path 
to the last non-duplicating path from the given paths array.


> How's change to: "The lead path is last tree path that was added or 
> set." to fully adapt to the doc.
>>
>> Could you also run the JTable and JTree JCK tests?
> 64 Tests all passed on Windows, report attached to the bug

    I think that we could ask the TCK team to add the test that checks 
DefaultTreeSelectionModel.setSelectionPaths() behaviour for the 
duplicating paths.


   Thanks,
   Alexandr.
>
>>
>> Thanks,
>> Alexandr.
>>
>> On 4/11/2014 3:24 AM, Vivi An wrote:
>>> Thanks Petr
>>>
>>> Please see comments inline. Plus, one more question, does new 
>>> package private function requires CCC approval?
>>>
>>> ~ Vivi
>>>
>>> On 4/10/2014 7:25 AM, Petr Pchelko wrote:
>>>> Hello, Vivi.
>>>>
>>>>>> Is it possible to write a test (manual or automated) for the fix?
>>>>> Two test files (One for JTree, one for JTable) attached to the bug 
>>>>> for manual test,  comments added in each file about the how to do 
>>>>> the test.
>>>> Is is possible to make an automatic test?
>>>> You could show the JTree, manually get it’s AccessibleContext and 
>>>> call methods that reproduce the problem.
>>>>
>>>> Adding a manual test to the JBS wouldn’t help, because nobody would 
>>>> ever run this test. So the best is to make an automatic jtreg test, 
>>>> or at least convert your manual tests to jtreg and push them into 
>>>> the repo together with the fix.
>>>>
>>>> With best regards. Petr.
>>> Discussed with SQE, auto test for this will require more 
>>> investigation, since it's not only a test to get access information 
>>> of a component,  in addition, it requires some human interaction, so 
>>> an autotest is possible with combine Robot+JTreg, since this target 
>>> to 14_03,  another bug JDK-8039978 is filed to track down the auto 
>>> test requirement for this, it's a good point since I will have more 
>>> of such fix coming up.
>>>>
>>>> 10 апр. 2014 г., в 6:04 после полудня, Vivi An <vivi.an at oracle.com> 
>>>> написал(а):
>>>>
>>>>> Updated webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~dmarkov/8036983/jdk9/webrev.01/
>>>>>
>>>>> Comments as below
>>>>>
>>>>> Thanks
>>>>>
>>>>> Vivi
>>>>>
>>>>>
>>>>>
>>>>> On 4/9/2014 8:08 AM, Alexander Scherbatiy wrote:
>>>>>> On 4/8/2014 9:19 PM, Vivi An wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you please review the fix for JDK 9?
>>>>>>>
>>>>>>> This bug is JAB related. ActivateDescenderPropertyChanged event 
>>>>>>> for JTree and JTable were not sent properly in case 
>>>>>>> SHIFT+CursorDown or Ctrl+CursorUp/Down are used. Fix made mainly 
>>>>>>> uses lead path (acctive child path) instead of selection path to 
>>>>>>> check if an event needs to be fired.
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8036983
>>>>>>> Webrev: http://cr.openjdk.java.net/~dmarkov/8036983/jdk9/webrev.00/
>>>>>>>
>>>>>> +        public void fireActiveDescendantPropertyChange(TreePath 
>>>>>> oldPath, TreePath newPath) {
>>>>>> Is it possible to make the method package access instead of public?
>>>>> Yes, good idea, fixed
>>>>>> +            int focusedRow = 
>>>>>> JTable.this.getSelectionModel().getLeadSelectionIndex();;
>>>>>> There is one more semicolon at the end.
>>>>>>
>>>>> Fixed
>>>>>> Is it possible to write a test (manual or automated) for the fix?
>>>>> Two test files (One for JTree, one for JTable) attached to the bug 
>>>>> for manual test,  comments added in each file about the how to do 
>>>>> the test.
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>> Thanks
>>>>>>>
>>>>>>> ~ Vivi
>>>
>>
>