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...@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
