On Fri, 6 Mar 2020 21:38:15 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Ajit Ghaisas has updated the pull request incrementally with one additional >> commit since the last revision: >> >> extra space removed > > 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. > > 1. I liked the approach proposed in PoC except the need to send > KeyMapping as a parameter to the test. It unnecessarily > makes test look complex. > We already have backward() and forward() helper functions in this test > class. The Key presses can easily be decided > based on orientation in these methods. I have modified the PoC test > provided by you to make it simpler. > Well, I disagree with your notion of "simpler" - replacing conditional blocks in tests by parameterized tests is the whole point of these. But at the end of the day, it's probably a matter of personal preferences :) > 2. Also, in PoC, testChangeOrientationSimpleForwardSelect does not > respect forward selection after Node orientation is > changed. The comment says it all - > `// same arrow as initial,now should have inverse effect` > The forward movement should still be forward even after Node > orientation change and not inverse. > the difference between my and your code in changeNodeOrientation is that mine only changes the table's orientation and yours additionally changes the parameter nodeOrientation - resulting in your navigational methods using keys based on the orientation while mine uses those for the old orientation. Changing the parameter smells, IMO. > > I have done modifications to the PoC code provided and added this test in > latest commit. > Please review. > Once we agree on the approach- > > * I will add other tests for horizontal keyMappings with modifiers to > TableViewHorizontalArrowsTest > > * I will revert changes done to TableViewKeyInputTest agreed on the overall approach, will add a couple of inline comments ------------- PR: https://git.openjdk.java.net/jfx/pull/114
