Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Mon, 17 Jun 2024 07:29:45 GMT, Tejesh R wrote: >> DetailsView removes JTable TAB, SHIFT-TAB, ENTER and SHIFT-ENTER >> functionalities to disable navigation within JTable of FilePane in >> DetailsView. This is causing the issue mentioned in the bug where on >> invoking DetailsView the functionalities are removed from JTable. I don't >> see it's effect/significance in disabling the navigation since the focus >> shifts outside the when TAB is presses and the folder opens when ENTER is >> pressed without this changed. >> I have tested the fix on CI system, its green. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Review fix - remove null initialization for table I think it is clear that modifying a shared map is a bad idea, so removing the code that is doing this is right. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19725#pullrequestreview-2131313263
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Tue, 18 Jun 2024 06:21:12 GMT, Tejesh R wrote: >> Yes, modifying the shared `ActionMap` is causing this issue though. As far >> as I have seen the copy first solution is mentioned in this bug >> https://bugs.openjdk.org/browse/JDK-8166352 as customer submitted workaround. > > I did thought of few solutions for this issue: > 1. To reset (If possible, but not sure how to do this, yet we have > `SwingUtilities.replaceUIActionMap`) the ActionMap. But when to reset is > again a question? > 2. To consider what customer has suggested about making a copy and then using > that which again I'm not sure since here shared defaults are used from > BasicTableUI. > 3. To remove the lines causing issue which I have proposed. I feel it is safe > now to remove it since TAB/ENTER functionalities (Basically TAB being moved > out of FilePane and ENTER on selecting file/opening Directory) is handled > without these lines too. I did CI test for any regression, but its look fine > without this lines too. TAB navigation is prevented by this fix https://bugs.openjdk.org/browse/JDK-4835633 where the TAB is rejected in [this](https://github.com/openjdk/jdk/blame/a81e1bf1e1a6f00280b9be987c03fe20915fd52c/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java#L683) line. - PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1647807264
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Mon, 17 Jun 2024 07:29:45 GMT, Tejesh R wrote: >> DetailsView removes JTable TAB, SHIFT-TAB, ENTER and SHIFT-ENTER >> functionalities to disable navigation within JTable of FilePane in >> DetailsView. This is causing the issue mentioned in the bug where on >> invoking DetailsView the functionalities are removed from JTable. I don't >> see it's effect/significance in disabling the navigation since the focus >> shifts outside the when TAB is presses and the folder opens when ENTER is >> pressed without this changed. >> I have tested the fix on CI system, its green. > > Tejesh R has updated the pull request incrementally with one additional > commit since the last revision: > > Review fix - remove null initialization for table Marked as reviewed by achung (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19725#pullrequestreview-2126571905
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Tue, 18 Jun 2024 05:13:14 GMT, Tejesh R wrote:
>> src/java.desktop/share/classes/sun/swing/FilePane.java line 1320:
>>
>>> 1318: }
>>> 1319:
>>> 1320:
>>> detailsTable.setFocusTraversalKeys(KeyboardFocusManager.FORWARD_TRAVERSAL_KEYS,
>>
>> If I have this right, JTable gets the map initialised via this code in
>> BasicTableUI.java
>>
>> InputMap getInputMap(int condition) {
>> if (condition == JComponent.WHEN_ANCESTOR_OF_FOCUSED_COMPONENT) {
>> InputMap keyMap =
>> (InputMap)DefaultLookup.get(table, this,
>> "Table.ancestorInputMap");
>> InputMap rtlKeyMap;
>>
>> if (table.getComponentOrientation().isLeftToRight() ||
>> ((rtlKeyMap = (InputMap)DefaultLookup.get(table, this,
>>
>> "Table.ancestorInputMap.RightToLeft")) == null)) {
>> return keyMap;
>> } else {
>> rtlKeyMap.setParent(keyMap);
>> return rtlKeyMap;
>> }
>> }
>> return null;
>> }
>>
>> This uses the shared defaults for the L&F.
>> I imagine that sharing is by far the most efficient thing to do for 99% of
>> uses, in which case I expect that somewhere there's some doc telling apps
>> that if they mess with the map for a component, they need to make a copy
>> first ? But how to do that ? And the author of the code above that modified
>> the shared map presumably was ignorant of this.
>
> Yes, modifying the shared `ActionMap` is causing this issue though. As far as
> I have seen the copy first solution is mentioned in this bug
> https://bugs.openjdk.org/browse/JDK-8166352 as customer submitted workaround.
I did thought of few solutions for this issue:
1. To reset (If possible, but not sure how to do this, yet we have
`SwingUtilities.replaceUIActionMap`) the ActionMap. But when to reset is again
a question?
2. To consider what customer has suggested about making a copy and then using
that which again I'm not sure since here shared defaults are used from
BasicTableUI.
3. To remove the lines causing issue which I have proposed. I feel it is safe
now to remove it since TAB/ENTER functionalities (Basically TAB being moved out
of FilePane and ENTER on selecting file/opening Directory) is handled without
these lines too. I did CI test for any regression, but its look fine without
this lines too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1643879140
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Mon, 17 Jun 2024 19:58:37 GMT, Phil Race wrote:
>> Tejesh R has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review fix - remove null initialization for table
>
> src/java.desktop/share/classes/sun/swing/FilePane.java line 1320:
>
>> 1318: }
>> 1319:
>> 1320:
>> detailsTable.setFocusTraversalKeys(KeyboardFocusManager.FORWARD_TRAVERSAL_KEYS,
>
> If I have this right, JTable gets the map initialised via this code in
> BasicTableUI.java
>
> InputMap getInputMap(int condition) {
> if (condition == JComponent.WHEN_ANCESTOR_OF_FOCUSED_COMPONENT) {
> InputMap keyMap =
> (InputMap)DefaultLookup.get(table, this,
> "Table.ancestorInputMap");
> InputMap rtlKeyMap;
>
> if (table.getComponentOrientation().isLeftToRight() ||
> ((rtlKeyMap = (InputMap)DefaultLookup.get(table, this,
>
> "Table.ancestorInputMap.RightToLeft")) == null)) {
> return keyMap;
> } else {
> rtlKeyMap.setParent(keyMap);
> return rtlKeyMap;
> }
> }
> return null;
> }
>
> This uses the shared defaults for the L&F.
> I imagine that sharing is by far the most efficient thing to do for 99% of
> uses, in which case I expect that somewhere there's some doc telling apps
> that if they mess with the map for a component, they need to make a copy
> first ? But how to do that ? And the author of the code above that modified
> the shared map presumably was ignorant of this.
Yes, modifying the shared `ActionMap` is causing this issue though. As far as I
have seen the copy first solution is mentioned in this bug
https://bugs.openjdk.org/browse/JDK-8166352 as customer submitted workaround.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1643800858
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Mon, 17 Jun 2024 07:29:45 GMT, Tejesh R wrote:
>> DetailsView removes JTable TAB, SHIFT-TAB, ENTER and SHIFT-ENTER
>> functionalities to disable navigation within JTable of FilePane in
>> DetailsView. This is causing the issue mentioned in the bug where on
>> invoking DetailsView the functionalities are removed from JTable. I don't
>> see it's effect/significance in disabling the navigation since the focus
>> shifts outside the when TAB is presses and the folder opens when ENTER is
>> pressed without this changed.
>> I have tested the fix on CI system, its green.
>
> Tejesh R has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review fix - remove null initialization for table
src/java.desktop/share/classes/sun/swing/FilePane.java line 1320:
> 1318: }
> 1319:
> 1320:
> detailsTable.setFocusTraversalKeys(KeyboardFocusManager.FORWARD_TRAVERSAL_KEYS,
If I have this right, JTable gets the map initialised via this code in
BasicTableUI.java
InputMap getInputMap(int condition) {
if (condition == JComponent.WHEN_ANCESTOR_OF_FOCUSED_COMPONENT) {
InputMap keyMap =
(InputMap)DefaultLookup.get(table, this,
"Table.ancestorInputMap");
InputMap rtlKeyMap;
if (table.getComponentOrientation().isLeftToRight() ||
((rtlKeyMap = (InputMap)DefaultLookup.get(table, this,
"Table.ancestorInputMap.RightToLeft")) == null)) {
return keyMap;
} else {
rtlKeyMap.setParent(keyMap);
return rtlKeyMap;
}
}
return null;
}
This uses the shared defaults for the L&F.
I imagine that sharing is by far the most efficient thing to do for 99% of
uses, in which case I expect that somewhere there's some doc telling apps that
if they mess with the map for a component, they need to make a copy first ? But
how to do that ? And the author of the code above that modified the shared map
presumably was ignorant of this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1643367580
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Mon, 17 Jun 2024 08:55:45 GMT, Prasanta Sadhukhan wrote: >> Yes, I hope now it is safe to remove these lines. Since TAB/SHIFT navigation >> is rejected with this >> [fix/line](https://github.com/openjdk/jdk/blame/5528ad74902fa4f4ec621d70e7e7d85f4ac1d780/src/java.desktop/share/classes/sun/swing/FilePane.java#L1314) >> which is further handled in BasicTableUI class >> ([Here](https://github.com/openjdk/jdk/blame/7b38bfea331437ad99277032de7fce939303abc8/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java#L679)). > > So far what I have seen in native windows filechooser, it seems TAB moves the > focus outside the table so long we are not editing the tablecell(to top of > Name column and then to textfield) in "Details" mode and NOT within the table > cells, which is what is being expected here... > If we are to follow the native behaviour, then I guess this should be closed > as "Not an issue" unless I am missing something.. It's not about `JFileChooser` TAB/ENTER key functionality here. The issue is w.r.t to JTable which when used/invoked after `JFileChooser` `DetailsView` is clicked. The functionality of TAB/ENTER which is supposed to move the focus to next column/row is been removed in `DetailsView` of `JFileChooser`. As in test case, I have added `JFileChooser` and `JTable` in a single frame. Once user clicks on `DetailsView` of `JFileChooser` and then tries to navigate within `JTable`, the functionality doesn't work since it's been removed from `UIActionMap`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1642466632
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Mon, 17 Jun 2024 07:24:32 GMT, Tejesh R wrote: >> src/java.desktop/share/classes/sun/swing/FilePane.java line 1321: >> >>> 1319: >>> 1320: // TAB/SHIFT-TAB should transfer focus and ENTER should >>> select an item. >>> 1321: // We don't want them to navigate within the table >> >> what was the original purpose of this code? it seems like the code was >> placed here for a reason but maybe it's no longer relevant? > > Yes, I hope now it is safe to remove these lines. Since TAB/SHIFT navigation > is rejected with this > [fix/line](https://github.com/openjdk/jdk/blame/5528ad74902fa4f4ec621d70e7e7d85f4ac1d780/src/java.desktop/share/classes/sun/swing/FilePane.java#L1314) > which is further handled in BasicTableUI class > ([Here](https://github.com/openjdk/jdk/blame/7b38bfea331437ad99277032de7fce939303abc8/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java#L679)). So far what I have seen in native windows filechooser, it seems TAB moves the focus outside the table so long we are not editing the tablecell(to top of Name column and then to textfield) in "Details" mode and NOT within the table cells, which is what is being expected here... If we are to follow the native behaviour, then I guess this should be closed as "Not an issue" unless I am missing something.. - PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1642451496
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
> DetailsView removes JTable TAB, SHIFT-TAB, ENTER and SHIFT-ENTER > functionalities to disable navigation within JTable of FilePane in > DetailsView. This is causing the issue mentioned in the bug where on invoking > DetailsView the functionalities are removed from JTable. I don't see it's > effect/significance in disabling the navigation since the focus shifts > outside the when TAB is presses and the folder opens when ENTER is pressed > without this changed. > I have tested the fix on CI system, its green. Tejesh R has updated the pull request incrementally with one additional commit since the last revision: Review fix - remove null initialization for table - Changes: - all: https://git.openjdk.org/jdk/pull/19725/files - new: https://git.openjdk.org/jdk/pull/19725/files/53440478..8adf43d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19725&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19725&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19725.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19725/head:pull/19725 PR: https://git.openjdk.org/jdk/pull/19725
Re: RFR: 6967482: TAB-key does not work in JTables after selecting details-view in JFileChooser [v2]
On Fri, 14 Jun 2024 20:17:29 GMT, Alisen Chung wrote:
>> Tejesh R has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review fix - remove null initialization for table
>
> src/java.desktop/share/classes/sun/swing/FilePane.java line 1321:
>
>> 1319:
>> 1320: // TAB/SHIFT-TAB should transfer focus and ENTER should select
>> an item.
>> 1321: // We don't want them to navigate within the table
>
> what was the original purpose of this code? it seems like the code was placed
> here for a reason but maybe it's no longer relevant?
Yes, I hope now it is safe to remove these lines. Since TAB/SHIFT navigation is
rejected with this
[fix/line](https://github.com/openjdk/jdk/blame/5528ad74902fa4f4ec621d70e7e7d85f4ac1d780/src/java.desktop/share/classes/sun/swing/FilePane.java#L1314)
which is further handled in BasicTableUI class
([Here](https://github.com/openjdk/jdk/blame/7b38bfea331437ad99277032de7fce939303abc8/src/java.desktop/share/classes/javax/swing/plaf/basic/BasicTableUI.java#L679)).
> test/jdk/javax/swing/JFileChooser/TABTestONFCExit.java line 54:
>
>> 52: */
>> 53:
>> 54: public class TABTestONFCExit {
>
> why is the test name capitalized like this?
Since TAB is Key representation, I have capitalized TAB, ON and FC
(FileChooser). Hope that should be ok.
> test/jdk/javax/swing/JFileChooser/TABTestONFCExit.java line 55:
>
>> 53:
>> 54: public class TABTestONFCExit {
>> 55: private static JTable table = null;
>
> does this jtable need to be initialized as null here?
Not mandatory though, can remove.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1642307944
PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1642310592
PR Review Comment: https://git.openjdk.org/jdk/pull/19725#discussion_r1642311240
