<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