RFR: 8204 : Keyboard traps in flamegraph view for search option [v2]
Virag Purnam
vpurnam at openjdk.org
Tue Feb 27 16:56:15 UTC 2024
On Mon, 26 Feb 2024 08:45:12 GMT, Brice Dutheil <bdutheil at openjdk.org> wrote:
>> Virag Purnam has updated the pull request incrementally with one additional commit since the last revision:
>>
>> 8204: Keyboard traps in flamegraph view for search option
>
> application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/manager/wizards/PresetManagerPage.java line 209:
>
>> 207:
>> 208: tableInspector.getViewer().refresh();
>> 209: tableInspector.setFocus();
>
> **suggestion:** Might be worth extracting all `setFocus()` statements in a single method with a comment, since the issue is not quite straightforward.
I have addressed this and extracted a method.
> application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 184:
>
>> 182: private AttributeSelection attributeSelection;
>> 183: private IToolBarManager toolBar;
>> 184: private static boolean traverseAlready = false;
>
> **question:** Why this flag needs to be `static` ?
Removed static.
> application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 519:
>
>> 517: flamegraphComponent.setFocusTraversalKeysEnabled(true);
>> 518:
>> 519: flamegraphComponent.addKeyListener(new KeyAdapter() {
>
> **suggestion:** Add a comment as what is the purpose of this listener, and why it's different from the other.
>
> Maybe extract in method as well.
Added comments and extracted to a method.
> application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 563:
>
>> 561: panel.setFocusTraversalKeysEnabled(true);
>> 562:
>> 563: panel.addKeyListener(new KeyAdapter() {
>
> **suggestion:** Add a comment as what is the purpose of this listener, and why it's different from the other.
>
> Maybe extract in method as well.
Extracted a method and added comments.
> application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 571:
>
>> 569: public void run() {
>> 570: setTraverseAlready(!traverseAlready);
>> 571: if (traverseAlready) {
>
> **suggestion:** Having a setter and a direct access to `traverseAlready` is a bit awkward. I suggest to either get rid of the setter, or add a getter.
>
> About this flag, the setter seems to only `toggle`, so if the method is kept I suggest instead to replace the setter by a `void toggleAlreadyTraversed() { traverseAlready = !traverseAlready }`.
Removed the setter method.
> application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 688:
>
>> 686: public void keyPressed(KeyEvent e) {
>> 687: if (e.getKeyCode() == KeyEvent.VK_TAB) {
>> 688: if (e.getModifiersEx() > 0) {
>
> **suggestion:** `e.getModifiersEx() > 0` comes up a lot, I believe the idea here is to handle <kbd>Shift</kbd> <kbd>Tab</kbd>, I believe it's the same on all OSes. Might be worth extracting this as util method.
Extracted a method and added comment.
> application/org.openjdk.jmc.flightrecorder.flamegraph/src/main/java/org/openjdk/jmc/flightrecorder/flamegraph/views/FlamegraphSwingView.java line 696:
>
>> 694: }
>> 695: }
>> 696: });
>
> **suggestion:** This looks like the same code as above I suggest extracting this to a method with some comment on the intent.
Extracted a method and added comments.
-------------
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1504614728
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1504616051
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1504621253
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1504620497
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1504619702
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1504618435
PR Review Comment: https://git.openjdk.org/jmc/pull/554#discussion_r1504617338
More information about the jmc-dev
mailing list