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

Add Ctrl-Shift-C shortcut #1476

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Add Ctrl-Shift-C shortcut #1476

merged 1 commit into from
Apr 25, 2019

Conversation

cwharvey
Copy link
Contributor

@cwharvey cwharvey commented Apr 20, 2019

Copies address at cursor from hexdump, graph and disassembly views

I don't really know Qt, so double check the new QShortcut usage

Test plan

Copied addresses from hex, graph and disassem views with both menu and shortcut

Closing issues

Fix #1474

@ITAYC0HEN
Copy link
Member

Thank you for you quick improvement with adding support for hex! :)

Next time, you don't have to close the previous PR, you can simply push changes to your branch, and it would immediately take place in the PR :)

For example, you could simply push your changes to "TheLorax:copy-address-shortcut"

@Maijin Maijin requested a review from ITAYC0HEN April 20, 2019 21:04
Copy link
Member

@ITAYC0HEN ITAYC0HEN left a comment

Choose a reason for hiding this comment

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

Please make Copy Address in the context menu (graph and disas), separated by a separator. And under "Copy" when available. As you can see in the image below, there is a separator under "Copy" and after it there is the "Copy Address". It makes more sense to pair them together to the same section of the context menu

image

Other than that, looks very good :)

@cwharvey
Copy link
Contributor Author

I don't normally use github, how do you update a pull request? Amend and force push? 2nd commit (ugly)?

@ITAYC0HEN
Copy link
Member

You simply need to push the changes from your local branch to the remote branch

$ cd cutter
$ git checkout hexdump-improvements
<do changes>
<commit changes>
$ git push 

Usually, what I do, from scratch is:

$ git clone https://github.com/radareorg/cutter
$ git remote add TheLorax https://github.com/TheLorax/cutter
$ git checkout -b my_new_branch
< do changes >
< commit changes >
$ git push TheLorax

Now you can go to https://github.com/TheLorax/cutter and make a pull request

If you want to add another commit to the PR, simply push again to your branch and the PR will be updated automatically:

< do changes >
< commit changes >
$ git push TheLorax

Copy link
Member

@xarkes xarkes left a comment

Choose a reason for hiding this comment

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

Hello,thank you for this Pull Request! :)

auto cpyAddrShortcut = new QShortcut(QKeySequence{Qt::CTRL + Qt::SHIFT + Qt::Key_C}, this);
cpyAddrShortcut->setContext(Qt::WidgetWithChildrenShortcut);
ui->actionCopyAddressAtCursor->setShortcut(Qt::CTRL + Qt::SHIFT + Qt::Key_C);
connect(cpyAddrShortcut, SIGNAL(activated()), this, SLOT(on_actionCopyAddressAtCursor_triggered()));
Copy link
Member

Choose a reason for hiding this comment

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

Just a reminder that new member pointer syntax for connecting signals and slots has been available for a while. connect(foo, &Foo::signalname, bar, &Bar::slotname). It has benefit of being checked at compile time. It's easy to silently break SIGNAL and SLOTS macros while refactoring.

Copy link
Member

Choose a reason for hiding this comment

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

@TheLorax can you please improve this Connect method as recommended by @karliss?

auto cpyAddrShortcut = new QShortcut(QKeySequence{Qt::CTRL + Qt::SHIFT + Qt::Key_C}, this);
cpyAddrShortcut->setContext(Qt::WidgetWithChildrenShortcut);
ui->actionCopyAddressAtCursor->setShortcut(Qt::CTRL + Qt::SHIFT + Qt::Key_C);
connect(cpyAddrShortcut, SIGNAL(activated()), this, SLOT(on_actionCopyAddressAtCursor_triggered()));
Copy link
Member

Choose a reason for hiding this comment

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

@TheLorax can you please improve this Connect method as recommended by @karliss?

 * Copies address at cursor from hexdump, graph and disassembly views
@xarkes
Copy link
Member

xarkes commented Apr 25, 2019

@ITAYC0HEN Maybe can you approve now?

@ITAYC0HEN ITAYC0HEN merged commit e00a70c into rizinorg:master Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Copy Address shortcut in hexdump
4 participants