<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