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

Fixed asm.bytes on by default in cursor mode #10157

Merged
merged 2 commits into from Jun 6, 2018

Conversation

cyanpencil
Copy link
Contributor

@cyanpencil cyanpencil commented May 22, 2018

Closes #9847

EDIT:

Remade this pr, now press # to toggle bytes when in disasm view.
No more turning on by default when switching to cursor.


OLD_PR:

When toggling the cursor, asm.bytes goes on and off.

However, if the user had specified before that e asm.bytes = true, then bytes are kept being shown even when exiting cursor mode.

This is done in a little hackish way:
asm.bytes = asm.bytes + 1 when switching to cursor mode
asm.bytes = asm.bytes - 1 when switching off cursor mode
(this means that asm.bytes = 2 when it was set by the user and then switched to cursor mode),
but doing like this avoids adding another config variable.

Tell me if it's too confusing.

@radare
Copy link
Collaborator

radare commented May 26, 2018

asm.bytes is boolean type, what about toggling it instead?

@radare
Copy link
Collaborator

radare commented May 26, 2018

anyway, maybe we should discuss for a better way to do that. because we want to get the bytes or not? because maybe the cursor in disasm without bytes should just scroll around instructions and ignore bytes :?

@cyanpencil
Copy link
Contributor Author

cyanpencil commented May 26, 2018

asm.bytes is boolean type, what about toggling it instead?

I decided not to toggle it because if the user wanted to see bytes in disasm without the cursor, then entering and exiting cursor mode will disable the bytes (and that would be something the user will not expect nor want). This way it "remembers" the previous state and won't disable bytes when exiting cursor mode if it was enabled before by the user.

However I understand that the implementation is a bit confusing, maybe I can change it by adding a config variable to be more clear in the code.

anyway, maybe we should discuss for a better way to do that. because we want to get the bytes or not? because maybe the cursor in disasm without bytes should just scroll around instructions and ignore bytes :?

Reading issue #9847 I assumed that bytes should be always on when entering cursor mode, but if I misunderstood tell me so I'll close this pr and continue discussion on the issue page.

@radare
Copy link
Collaborator

radare commented May 28, 2018

maybe its better to add a key to toggle the bytes instead of making this behaviour by default. what do you think?

@cyanpencil
Copy link
Contributor Author

Imho that's not a bad idea, we could use for example I key to toggle bytes (both in visual and in graph), and leave them on by default like they are right now.

@radare
Copy link
Collaborator

radare commented May 31, 2018

I is already used. but not in the help message of V?

@cyanpencil
Copy link
Contributor Author

I is not listed in the V? help maybe because it is an alias of i...?

Anyway. do you have any suggestions? Maybe a special character like ~ or ', or even #?

@radare
Copy link
Collaborator

radare commented May 31, 2018 via email

@radare
Copy link
Collaborator

radare commented May 31, 2018 via email

Fix whitespace in visual.c

Refactoring cursor in visual.c

Revert asm.bytes false by default
@@ -2484,6 +2486,9 @@ R_API int r_core_visual_cmd(RCore *core, const char *arg) {
case ')':
rotateAsmemu (core);
break;
case '#':
r_core_cmd0 (core, "e!asm.bytes");
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the api: r_config_toggle(core->config, "asm.bytes");

@radare
Copy link
Collaborator

radare commented Jun 6, 2018

sorry for my late review. change this and i'll merge. thanks!!

@radare radare merged commit dcaeb35 into radareorg:master Jun 6, 2018
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