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

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Tue Apr 15 11:36:16 UTC 2014


On 4/15/2014 10:44 AM, Vivi An wrote:
> Hi Alex,
>
> On 4/14/2014 5:59 AM, Alexander Scherbatiy wrote:
>> 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.
>>
> Now I understand what you mean by "ambiguous" :).  Yes, it is. Even we 
> update the setSelectionPaths API doc to something like "The lead path 
> is set to the last unique pathin pPaths".   The lead path set through 
> setSelectionPaths will still not match the JDK doc you mentioned (The 
> lead TreePath is the last path that was added (or set) in all scenarios:
>
> =============================
> For example:
> In case user making multi-selection of nodes through <Shift+ Cursor 
> Up>, if user selected a tree node "path3" first, pressed <Shift+Cursor 
> up> to select other tree nodes: <path2>, then <path1>, the lead path 
> is always  set to "path3" in setSelectionPaths  because it's always 
> the last path in the passed in pPaths - which I think is not right. 
> That's why I made the fix in.  The fix will make the lead path to be 
> the last path that was added (set), as mentioned in the JDK doc,  but 
> yes, this can break existing application because it's behavior change.
>
> Anyway, we don't have to make change in setSelectionPaths for 
> 8036983.  Since in JTree.java, we now use setLeadSelectionPath to fire 
> active descendant property change instead of valueChange, which is 
> eventually triggered through key event processing and 
> BasicTreeUI::extendSelection with the lead path (Active child) we 
> expected.
>
> Reverted the change in DefaultTreeSelectionModel.java, keep all other 
> changes in JTree.java and JTable.java, JDK-8040209 
> <https://bugs.openjdk.java.net/browse/JDK-8040209>was filed so we can 
> track the lead path or API doc in 
> DefaultTreeSelectionModel.setSelectionPaths separately.  Re-tested 
> manual test cases attached to the bug and re-ran JCK tests, tests passed.

      Could you send the updated webrev where the 
DefaultTreeSelectionModel is not changed to review?

     Thanks,
     Alexandr.
>
>>
>>> 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.
> JCK-7302801 <https://bugs.openjdk.java.net/browse/JCK-7302801> is 
> filed to track this down.
>
> Thank you
>
> ~ Vivi
>>
>>   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
>>>>>
>>>>
>>>
>>
>




More information about the swing-dev mailing list