Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to 62336 #396

Closed
wants to merge 2 commits into from
Closed

Fix to 62336 #396

wants to merge 2 commits into from

Conversation

@varan123
Copy link

@varan123 varan123 commented Aug 26, 2018

Description

Keyboard shortcut Ctrl +6 is not working.
Was reproduced for Java 8 on Windows 10.
actionEvent.getActionCommand() in MainFrame unexpected returning null only for CTRL-6. getActionCommand was replaced with alternative code. New version working stable.

Motivation and Context

All CTRL-{N} working stable, after fix.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62336

How Has This Been Tested?

Tested on Java 8 and Java 9 on Windows 10. Issue was reproduced. New code working fine.

Screenshots (if appropriate):

Types of changes

  • Bug fix 62336. Was changed org.apache.jmeter.gui.MainFarme around line 686.

Checklist:

Path Filename Extension Status Lines added Lines removed Last Modified Size
src/core/org/apache/jmeter/gui/MainFrame.java MainFrame.java .java Modified 12 1

@codecov-io
Copy link

@codecov-io codecov-io commented Aug 26, 2018

Codecov Report

Merging #396 into trunk will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk     #396      +/-   ##
============================================
- Coverage     58.62%   58.61%   -0.01%     
  Complexity    10602    10602              
============================================
  Files          1193     1193              
  Lines         75837    75842       +5     
  Branches       7330     7331       +1     
============================================
  Hits          44457    44457              
- Misses        28869    28874       +5     
  Partials       2511     2511
Impacted Files Coverage Δ Complexity Δ
src/core/org/apache/jmeter/gui/MainFrame.java 1.53% <0%> (-0.03%) 1 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8fb20a...5be6819. Read the comment docs.

Copy link
Contributor

@FSchumacher FSchumacher left a comment

Thanks for your patch.
I have one question and a few minor notes about code formatting.

@@ -680,7 +683,15 @@ private void addQuickComponentHotkeys(JTree treevar) {

@Override
public void actionPerformed(ActionEvent actionEvent) {
String propname = "gui.quick_" + actionEvent.getActionCommand();
//Bug 62336
AWTEvent current_event = EventQueue.getCurrentEvent();
Copy link
Contributor

@FSchumacher FSchumacher Aug 26, 2018

Have you checked what actionEvent is here? If I read the Swing-API correctly, it should be the same object as current_event. If actionEvent is really null here, is it only sometimes null and the NPE stops the AWT thread?
As a minor note: current_event should be written in camel case as currentEvent to match the other names.

//Bug 62336
AWTEvent current_event = EventQueue.getCurrentEvent();
String key_text = "";
if(current_event instanceof KeyEvent) {
Copy link
Contributor

@FSchumacher FSchumacher Aug 26, 2018

I would place a space between if and the opening parenthesis as if is not a function call.

String key_text = "";
if(current_event instanceof KeyEvent) {
KeyEvent key_event = (KeyEvent)current_event;
key_text = KeyEvent.getKeyText( key_event.getKeyCode() );
Copy link
Contributor

@FSchumacher FSchumacher Aug 26, 2018

Again a minor nit: No space needed after the opening and before the closing parenthesis as this is a function call.
And as above variable names should be written in camel case: keyEvent instead of key_event.

AWTEvent current_event = EventQueue.getCurrentEvent();
String key_text = "";
if(current_event instanceof KeyEvent) {
KeyEvent key_event = (KeyEvent)current_event;
Copy link
Contributor

@FSchumacher FSchumacher Aug 26, 2018

Here I would put a space after the closing parenthesis as this is a cast.

@varan123
Copy link
Author

@varan123 varan123 commented Aug 26, 2018

Implemented few code style recommendations from Felix Schumacher.
@FSchumacher
Copy link
Contributor

@FSchumacher FSchumacher commented Aug 29, 2018

Right, the events will never be the same, but still... I wonder why ActionEvent is null in the first place. This seems to be a bug in the JDK, it doesn't make sense to call an action listener without a valid event.
Can you post a stacktrace when the event is null (and maybe return early from the function and see, if the next event in the action listener is the correct one)?

@varan123
Copy link
Author

@varan123 varan123 commented Aug 31, 2018

I based my solution on discussion: https://stackoverflow.com/questions/32978936/why-actionevent-getactioncommand-returns-null
If we want to receive key value, have sense to receive it directly from KeyEvent.
Debug results:
Ctrl-6: propname value "gui.quick_null"
Ctrl-3: propname value "gui.quick_3"
Stacktrace:
at org.apache.jmeter.gui.MainFrame$2.actionPerformed(MainFrame.java:696)
at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1663)
at javax.swing.JComponent.processKeyBinding(JComponent.java:2882)
at javax.swing.KeyboardManager.fireBinding(KeyboardManager.java:307)
at javax.swing.KeyboardManager.fireKeyboardAction(KeyboardManager.java:250)
at javax.swing.JComponent.processKeyBindingsForAllComponents(JComponent.java:2974)
at javax.swing.JComponent.processKeyBindings(JComponent.java:2966)
at javax.swing.JComponent.processKeyEvent(JComponent.java:2845)
at java.awt.Component.processEvent(Component.java:6310)
at java.awt.Container.processEvent(Container.java:2237)
at java.awt.Component.dispatchEventImpl(Component.java:4889)
at java.awt.Container.dispatchEventImpl(Container.java:2295)
at java.awt.Component.dispatchEvent(Component.java:4711)
at java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1954)
at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(DefaultKeyboardFocusManager.java:806)
at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(DefaultKeyboardFocusManager.java:1074)
at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:945)
at java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:771)
at java.awt.Component.dispatchEventImpl(Component.java:4760)
at java.awt.Container.dispatchEventImpl(Container.java:2295)
at java.awt.Window.dispatchEventImpl(Window.java:2746)
at java.awt.Component.dispatchEvent(Component.java:4711)
at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:758)
at java.awt.EventQueue.access$500(EventQueue.java:97)
at java.awt.EventQueue$3.run(EventQueue.java:709)
at java.awt.EventQueue$3.run(EventQueue.java:703)
at java.security.AccessController.doPrivileged(AccessController.java)
at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:80)
at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:90)
at java.awt.EventQueue$4.run(EventQueue.java:731)
at java.awt.EventQueue$4.run(EventQueue.java:729)
at java.security.AccessController.doPrivileged(AccessController.java)
at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:80)
at java.awt.EventQueue.dispatchEvent(EventQueue.java:728)
at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:201)
at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116)
at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93)
at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)

@FSchumacher
Copy link
Contributor

@FSchumacher FSchumacher commented Sep 10, 2018

Just wanted to let you know, that you are not forgotten. I have now access to a windows 10 system and can reproduce the issue. But before I implement your fix, I would like to understand why ctrl+6doesn't work, as all other keys seem to work OK.

@asfgit asfgit closed this in 4970be0 Oct 28, 2018
@FSchumacher
Copy link
Contributor

@FSchumacher FSchumacher commented Oct 28, 2018

Thanks for your contribution.

I still don't know, why we get null for CTRL+6 on windows, but the workaround seems to work, so I have included it and hope somebody can explain the real bug to me.

asfgit pushed a commit that referenced this issue Oct 28, 2018
With the changes for the original bug report, the inner class
got too big, so refactor it out into a nested static class.

Some shortcuts are not working correctly on windows
Contributed by Michael Pavlov (michael.paulau at gmail.com)

Followup to r1845017
Bugzilla Id: 62336
Relates to #396 on github



git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1845021 13f79535-47bb-0310-9956-ffa450edef68
hitesh22 pushed a commit to hitesh22/jmeter that referenced this issue Nov 15, 2018
Contributed by Michael Pavlov (michael.paulau at gmail.com)

Bugzilla Id: 62336
Closes apache#396 on github


git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1845017 13f79535-47bb-0310-9956-ffa450edef68
hitesh22 pushed a commit to hitesh22/jmeter that referenced this issue Nov 15, 2018
With the changes for the original bug report, the inner class
got too big, so refactor it out into a nested static class.

Some shortcuts are not working correctly on windows
Contributed by Michael Pavlov (michael.paulau at gmail.com)

Followup to r1845017
Bugzilla Id: 62336
Relates to apache#396 on github



git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1845021 13f79535-47bb-0310-9956-ffa450edef68
StorDm pushed a commit to etnetera/jmeter that referenced this issue Jan 7, 2021
Contributed by Michael Pavlov (michael.paulau at gmail.com)

Bugzilla Id: 62336
Closes apache#396 on github


git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1845017 13f79535-47bb-0310-9956-ffa450edef68

Former-commit-id: 4970be0
StorDm pushed a commit to etnetera/jmeter that referenced this issue Jan 7, 2021
With the changes for the original bug report, the inner class
got too big, so refactor it out into a nested static class.

Some shortcuts are not working correctly on windows
Contributed by Michael Pavlov (michael.paulau at gmail.com)

Followup to r1845017
Bugzilla Id: 62336
Relates to apache#396 on github



git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1845021 13f79535-47bb-0310-9956-ffa450edef68

Former-commit-id: c9fc78c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants