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

Added Shortcut key event for Reset search - 66053 #710

Merged
merged 1 commit into from
May 19, 2023
Merged

Added Shortcut key event for Reset search - 66053 #710

merged 1 commit into from
May 19, 2023

Conversation

rollno748
Copy link
Contributor

Description

Shortcut key to Reset Search

Motivation and Context

This shortcut will help to Reset Search while enhancing or debugging the script

How Has This Been Tested?

Executed Unit test on core
Validated the shortcut in local - works perfectly!

Screenshots (if appropriate):

image

Types of changes

  • Adding Keyevent to enable shortcut for reset search functionality (non-breaking change which uses existing functionality)

Checklist:

  • My code follows the [code style][style-guide] of this project.
  • I have updated the documentation accordingly.

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Merging #710 (2e0d9df) into master (a997ed2) will decrease coverage by 0.00%.
The diff coverage is 22.22%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #710      +/-   ##
============================================
- Coverage     55.23%   55.22%   -0.01%     
  Complexity    10382    10382              
============================================
  Files          1061     1061              
  Lines         65763    65771       +8     
  Branches       7532     7531       -1     
============================================
+ Hits          36322    36323       +1     
- Misses        26841    26848       +7     
  Partials       2600     2600              
Impacted Files Coverage Δ
...org/apache/jmeter/gui/action/SearchTreeDialog.java 6.14% <0.00%> (-0.19%) ⬇️
.../java/org/apache/jmeter/gui/action/KeyStrokes.java 91.80% <100.00%> (+0.13%) ⬆️
...java/org/apache/jmeter/gui/util/JMeterMenuBar.java 78.25% <100.00%> (ø)
...isualizers/backend/influxdb/HttpMetricsSender.java 78.12% <0.00%> (-2.09%) ⬇️
.../apache/jmeter/threads/openmodel/scheduleParser.kt 69.60% <0.00%> (ø)
...a/org/apache/jmeter/timers/PoissonRandomTimer.java 78.37% <0.00%> (+5.40%) ⬆️

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 a997ed2...2e0d9df. Read the comment docs.

@rollno748
Copy link
Contributor Author

rollno748 commented May 7, 2022

@vlsi - can you please review and approve
Its just a minor change in adding shortcut

Also, added a reset search button to search dialog

@pmouawad
Copy link
Contributor

pmouawad commented May 9, 2022

Hello @rollno748 ,
Thanks for contributing.
Can you run ./gradlew autostyleApply and commit the result please ?

Thank you

@rollno748
Copy link
Contributor Author

Thanks for reviewing it @pmouawad
I have applied the autostyle and committed it

@vlsi
Copy link
Collaborator

vlsi commented May 9, 2022

Am I right the suggested shortcut is ctrl+alt+f to reset the search?
Does it come from a standard?

Have you considered using esc shortcut to reset the search?

@rollno748
Copy link
Contributor Author

rollno748 commented May 9, 2022

Yes the current shortcut is assigned to ctrl+alt+f to reset the search.
I haven't considered the esc to set it as reset search.
I thought, It would be easier for us to remember to go with the same keys used to search (ctrl + f) - with an alternate key added for reset (ctrl + alt + f)

Happy to hear your thoughts as well
please advise !

@vlsi
Copy link
Collaborator

vlsi commented May 10, 2022

Frankly speaking, I think we should try using ESC shortcut for "resetting the search"

@rollno748
Copy link
Contributor Author

rollno748 commented May 10, 2022

Cool, Thanks for the inputs.
I have verified the KeyStrokes.java -> It appears that the key is already assigned
Upon checking the reference, its been used in the save property dialog.

@vlsi Do you still want me to assign the same key to the reset search ?

@vlsi vlsi added this to the 5.6 milestone May 19, 2023
@vlsi vlsi merged commit ebc1615 into apache:master May 19, 2023
@vlsi
Copy link
Collaborator

vlsi commented May 19, 2023

@rollno748 , thanks. For the next PR, please create a different branch. It is not a good idea to create PRs from master branch. For instance, in that case you won't be able to create two PRs at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants