On Fri, 6 Mar 2020 11:47:42 GMT, Ajit Ghaisas <aghai...@openjdk.org> wrote:
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8235480 >> >> Fix : Added the missed out RTL checks to the key mappings in >> TableViewBehaviorBase class. >> >> Testing : Modified unit tests in TableViewKeyInputTest to take orientation >> as a parameter. The Left/Right key press tests have been modified to address >> LTR and RTL orientations. >> >> Note : If this test modification is acceptable, I would like to address >> other similar tests separately. (I will create a test JBS issue and address >> later) > > The pull request has been updated with 1 additional commit. I think the fix looks good and the approach you took for the new test looks good to me. If @kleopatra is OK with it, then please proceed. I left a couple minor comments. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TableViewBehaviorBase.java line 150: > 149: new KeyMapping(LEFT, e -> { if(isRTL()) > selectRightCell(); else selectLeftCell(); }), > 150: new KeyMapping(KP_LEFT,e -> { if(isRTL()) > selectRightCell(); else selectLeftCell(); }), > 151: new KeyMapping(RIGHT, e -> { if(isRTL()) > selectLeftCell(); else selectRightCell(); }), OK, I checked into this a little more closely and I can't think if a cleaner way to do it. What I originally suggested (using a ternary operator) doesn't work. And the other thing I was going to suggest, that of creating new methods like `selectForwardCell` (kind of like you did in the test) isn't worth the effort, since each is only repeated twice and it would likely result in code that isn't any easier to understand or maintain. So I think what you've done is fine. Btw, I guess you didn't mean to remove the space after the `,` here? modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewHorizontalArrowsTest.java line 202: > 201: > 202: // Now, test that the forward select resprects change in > NodeOrientation > 203: forward(); Typo: should be "respects" modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewHorizontalArrowsTest.java line 51: > 50: /** > 51: * Test basic horizontal navigation mappings for TableView. It's > parametrized on NodeOrientation > 52: */ Typo: should be parameterized modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewHorizontalArrowsTest.java line 219: > 218: > 219: // Now, test that the backward select resprects change in > NodeOrientation > 220: backward(); "respects" ------------- PR: https://git.openjdk.java.net/jfx/pull/114
