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

Allow copy operations to work on multiple selected functions #28

Conversation

paulsapps
Copy link
Contributor

Allows "copy name", "copy address" to work with multiple selected functions. Also adds "copy name and address" for single/multiple functions.

Copy link
Owner

@gaasedelen gaasedelen left a comment

Choose a reason for hiding this comment

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

a few nitpicks on code style & readability

gui_rename_function(function_addresses[0])

# handle the 'Copy name and address' or 'Copy names and addresses' action
elif action == self._action_copy_name_and_address or action == self._action_copy_names_and_addresses:
Copy link
Owner

Choose a reason for hiding this comment

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

Please change to:

elif action in [self._action_copy_name_and_address, self._action_copy_names_and_addresses]:
    ...

copy_to_clipboard(function_name_and_address)

# handle the 'Copy name' or 'Copy names' action
elif action == self._action_copy_name or action == self._action_copy_names:
Copy link
Owner

Choose a reason for hiding this comment

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

Same as comment above,

elif action in [self._action_copy_name, self._action_copy_names]:
    ...

elif action == self._action_copy_address:
address_string = "0x%X" % function_address
# handle the 'Copy address' or 'Copy addresses' action
elif action == self._action_copy_address or action == self._action_copy_addresses:
Copy link
Owner

Choose a reason for hiding this comment

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

Same as the other two comments!

@@ -327,9 +327,14 @@ def _ui_init_ctx_menu_actions(self):

# function actions
self._action_rename = QtWidgets.QAction("Rename", None)
self._action_copy_name_and_address = QtWidgets.QAction("Copy name and address", None)
Copy link
Owner

Choose a reason for hiding this comment

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

Move this action/member definition to be after self._action_copy_address

self._action_copy_name = QtWidgets.QAction("Copy name", None)
self._action_copy_address = QtWidgets.QAction("Copy address", None)

self._action_copy_names_and_addresses = QtWidgets.QAction("Copy names and addresses", None)
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above, move this definition after self._action_copy_addresses

@gaasedelen
Copy link
Owner

This seems like a reasonable request to me :-)

I've made a few nitpicks on code style and readability, but nothing major. I'd also like you to make the PR to the develop branch where I work on vNext / new features. Master is reserved for major releases or hotfixes.

Thanks for the PR!

@paulsapps paulsapps force-pushed the feature_copy_multiple_function_name_and_addresses branch from 9041ccf to f5c657e Compare December 18, 2017 00:44
@paulsapps paulsapps force-pushed the feature_copy_multiple_function_name_and_addresses branch from f5c657e to 3024fd2 Compare December 18, 2017 00:51
@paulsapps paulsapps changed the base branch from master to develop December 18, 2017 00:52
@paulsapps
Copy link
Contributor Author

Fixed.. I think :)?

@gaasedelen
Copy link
Owner

Thanks, looks good to me 👍

@gaasedelen gaasedelen merged commit f2dff09 into gaasedelen:develop Dec 18, 2017
@paulsapps paulsapps deleted the feature_copy_multiple_function_name_and_addresses branch December 18, 2017 13:11
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.

None yet

2 participants