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.

@cyanpencil cyanpencil force-pushed the cyanpencil:fix-visual-cursor-bytes branch from 0d33dc5 to 521d4eb May 22, 2018
@radare
Copy link
Collaborator

@radare radare commented May 26, 2018

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

@radare
Copy link
Collaborator

@radare 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 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 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

@cyanpencil cyanpencil commented May 28, 2018

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 radare commented May 31, 2018

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

@cyanpencil
Copy link
Contributor Author

@cyanpencil cyanpencil commented May 31, 2018

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 radare commented May 31, 2018

@radare
Copy link
Collaborator

@radare radare commented May 31, 2018

Fix whitespace in visual.c

Refactoring cursor in visual.c

Revert asm.bytes false by default
@cyanpencil cyanpencil force-pushed the cyanpencil:fix-visual-cursor-bytes branch from 521d4eb to ee2dfc5 Jun 2, 2018
@@ -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");

This comment has been minimized.

@radare

radare Jun 6, 2018
Collaborator

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

@radare
Copy link
Collaborator

@radare 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
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants