<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 13:14:08 UTC 2014
The fix looks good for me.
Thanks,
Alexandr.
On 4/15/2014 4:45 PM, Vivi An wrote:
> 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