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 new menu option under 'Help' for Keyboard-Shortcuts. #3176

Merged
merged 1 commit into from
Mar 28, 2020

Conversation

alekhgupta1441
Copy link
Contributor

Fixes #3148
Screenshot from 2020-02-07 01-18-05
Included keyboard shortcuts in 'Help' menu.

@alekhgupta1441 alekhgupta1441 changed the title Extended the 'Help' menu. Fixes #3148 Extended the 'Help' menu. Feb 6, 2020
@devnulling
Copy link
Member

@alekhgupta1441 thanks for the PR.

A few things:

  1. we will need a CLA from you.

https://github.com/gnuradio/gnuradio/blob/master/CONTRIBUTING.md
https://wiki.gnuradio.org/index.php/Development#What.27s_this_Copyright_Assignment.3F

In order to get the process rolling, please email "bhilburn AT gnuradio.org" for a form to start the copyright process.

  1. You should only have a single commit for this change, you will need to squash the multiple commits into one.

  2. This change adds a little bit too much text to that display, so on smaller screens, like what you have a screen shot of, the "close" menu will not be displayed.

i think it would be better to add a new menu option under "Help" such as "Keyboard Shortcuts" that would open up a different alert/popup window, where this info would be at.

@devnulling
Copy link
Member

One more request, the commit message should prefix the section of code that it changes for example:

grc: Extend the help menu

@alekhgupta1441
Copy link
Contributor Author

I have mailed for CLA prcoess.
And i will soon update this pr with requested changes.

Copy link
Contributor

@michaelld michaelld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@777arc
Copy link
Member

777arc commented Feb 9, 2020

Sounds like @alekhgupta1441 wants to update it to address the small screen size issue, and we're also waiting on the CLA, but should be good to go after that.

@alekhgupta1441 alekhgupta1441 force-pushed the alekh_gr branch 2 times, most recently from cd2e01d to f316b35 Compare March 3, 2020 21:06
@alekhgupta1441
Copy link
Contributor Author

alekhgupta1441 commented Mar 3, 2020

Sorry, for being quite late.
I have updated the PR with requested changes.
And got CLA processed.

@alekhgupta1441 alekhgupta1441 changed the title Extended the 'Help' menu. Added new menu option under 'Help' for Keyboard-Shortcuts. Mar 4, 2020
@alekhgupta1441
Copy link
Contributor Author

alekhgupta1441 commented Mar 4, 2020

New menu screenshot.
Ctrl+K is short-cut to this menu.
Screenshot from 2020-03-04 23-33-23
CLA is signed.

@alekhgupta1441
Copy link
Contributor Author

@michaelld,@marcusmueller,@devnulling, @777arc can you please review this?

@777arc
Copy link
Member

777arc commented Mar 9, 2020

@alekhgupta1441 I don't see the keyboard shortcuts menu, is it supposed to be listed under Help? Control-K doesn't do anything for me either.

-Marc

@alekhgupta1441
Copy link
Contributor Author

alekhgupta1441 commented Mar 9, 2020

@777arc Yes, Keys menu option is under 'Help' and CTRL +K opens Keyboard Shortcuts menu.I checked it again by building gnuradio from branch alekh_gr.

@alekhgupta1441
Copy link
Contributor Author

WhatsApp Image 2020-03-09 at 3 56 15 PM
Keys menu.

@777arc
Copy link
Member

777arc commented Mar 9, 2020

Oops had PYTHONPATH issues. Yeah looks good!

@mormj mormj removed the Needs CLA label Mar 20, 2020
@nickoe nickoe self-requested a review March 28, 2020 10:44
Copy link
Contributor

@nickoe nickoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks safe to me.

Copy link
Contributor

@michaelld michaelld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaelld
Copy link
Contributor

All looks good here: CLA; multiple approvals; relatively simple And useful addition. Merging.

@michaelld michaelld merged commit 3867f4b into gnuradio:master Mar 28, 2020
@alekhgupta1441
Copy link
Contributor Author

@777arc , @michaelld, @nickoe Thanks for Reviewing and Merging PR.

@nickoe
Copy link
Contributor

nickoe commented Mar 28, 2020

@michaelld @alekhgupta1441 @marcusmueller Do we want this on maint-3.8 as well, or should we ignore that?

@nickoe
Copy link
Contributor

nickoe commented Mar 30, 2020

I just tested this as a cherry-pick on maint-3.8 and it seem to work well. But this is only testing with python3.

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.

GRC: Help Menu should be extended
7 participants