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

Vivi An vivi.an at oracle.com
Tue Apr 15 12:45:01 UTC 2014


Updated Webrev:

http://cr.openjdk.java.net/~dmarkov/8036983/jdk9/webrev.02/

Thanks

~ VIvi
On 4/15/2014 4:36 AM, Alexander Scherbatiy wrote:
> 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