RFR: 8204 : Keyboard traps in flamegraph view for search option [v2]

[Virag Purnam]

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