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

Offset history #9591

Merged
merged 12 commits into from Mar 8, 2018

Conversation

Projects
None yet
4 participants
@cyanpencil
Copy link
Contributor

cyanpencil commented Mar 5, 2018

Closes #6756.

When pressing the arrow keys while in "offset mode" (after pressing 'o' in visual) the history will only show seek commands, and truncate them to show only the offset.

Before: asciinema
After: asciinema

Inside r_line_hist_up(), to check if r2 is in offset mode, I used
if (strcmp(r_line_get_prompt(), "[offset]> ") == 0) {...}
I know it's ugly, but it was just a test to see if it worked, as this is my first pull request here and I didn't want to change too much code without advice.

So, do you have any suggestions on how could I check more correctly if r2 is in "offset mode" or not?
I was thinking about adding a flag like offset_prompt to struct RLine, and then doing
if (I.offset_prompt) {...}
to keep things as simple as possible, but please tell me if you have better ideas.

Feel free to tell me if it's wrong, I still have a lot to learn...

@@ -313,6 +313,13 @@ static int r_line_hist_up() {
if (!I.history.data) {
inithist ();
}
if (strcmp(r_line_get_prompt(), "[offset]> ") == 0) {

This comment has been minimized.

@radare

radare Mar 5, 2018

Owner

Use ! Instead of ==0 and add space before (

This comment has been minimized.

@radare

radare Mar 5, 2018

Owner

Uhm this strcmp is kind of a hack, prompt shouldnt change the behaviour

@radare

This comment has been minimized.

Copy link
Owner

radare commented Mar 5, 2018

We should use a different history for the offset and the shell prompt, or just disable the history to avoid unexpected behaviour,

Also, theres an issue discussing about removing Vo because i odnt know how many people use it. I mainly do V:s...

@cyanpencil

This comment has been minimized.

Copy link
Contributor Author

cyanpencil commented Mar 5, 2018

We should use a different history for the offset and the shell prompt, or just disable the history to avoid unexpected behaviour

What about using the seek history? (The one that can be scrolled using s+ and s-)

Uhm this strcmp is kind of a hack, prompt shouldnt change the behaviour

I agree completely with that, in fact in the PR statement I suggested to add a boolean flag to control this behaviour, but I was waiting for the opinion of a more expert developer.

Also, theres an issue discussing about removing Vo because i odnt know how many people use it. I mainly do V:s...

If you think this command is likely going to be removed, please state so because than this PR is useless and I'll close it.

In the meantime I fixed according to your reviews.

@radare

This comment has been minimized.

Copy link
Owner

radare commented Mar 5, 2018

@XVilka

This comment has been minimized.

Copy link
Collaborator

XVilka commented Mar 6, 2018

Using "o" is shorter and with this just seek history will be useful to filter out unneeded things. I think we can keep that, afair @Maijin is also using 'Vo'

@Maijin

This comment has been minimized.

Copy link
Collaborator

Maijin commented Mar 6, 2018

Yeah Vo is nice

@XVilka

This comment has been minimized.

Copy link
Collaborator

XVilka commented Mar 7, 2018

Also while you are on it - can you please add also the TAB auto-completion for the offsets, like it is done for s (seek) command?

@radare

This comment has been minimized.

Copy link
Owner

radare commented Mar 7, 2018

@cyanpencil

This comment has been minimized.

Copy link
Contributor Author

cyanpencil commented Mar 7, 2018

Sorry for the silence, I had some urgent college homework.

I'll start working on it in a couple of hours, I think I'll finish before tomorrow.

Also while you are on it - can you please add also the TAB auto-completion for the offsets, like it is done for s (seek) command?

No problem 👍

@cyanpencil

This comment has been minimized.

Copy link
Contributor Author

cyanpencil commented Mar 8, 2018

I think I have finished.
The offset prompt has the same exact autocompletion of the s command, and its history is the seek history.
I added two fields to the RLine struct to achieve this: the flag offset_prompt and the int offset_index to keep track how much back in the history the user is going. I hope it's not too much clutter added.

@XVilka

This comment has been minimized.

Copy link
Collaborator

XVilka commented Mar 8, 2018

In file included from line.c:41:0:
dietline.c: In function ‘r_line_hist_up’:
dietline.c:324:23: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘RIOUndos {aka struct r_io_undos_t}’ [-Wformat=]
         sprintf(line, "0x%"PFMT64x, undo->seek[undo->idx + I.offset_index]);
                       ^~~~~         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/akochkov/radare/radare2/libr/include/r_cons.h:10:0,
                 from line.c:3:
/home/akochkov/radare/radare2/libr/include/r_types.h:360:20: note: format string is defined here
 #define PFMT64x "llx"
In file included from line.c:41:0:
dietline.c: In function ‘r_line_hist_down’:
dietline.c:354:23: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘RIOUndos {aka struct r_io_undos_t}’ [-Wformat=]
         sprintf(line, "0x%"PFMT64x, undo->seek[undo->idx + I.offset_index]);
                       ^~~~~         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/akochkov/radare/radare2/libr/include/r_cons.h:10:0,
                 from line.c:3:
/home/akochkov/radare/radare2/libr/include/r_types.h:360:20: note: format string is defined here
@XVilka

This comment has been minimized.

Copy link
Collaborator

XVilka commented Mar 8, 2018

And one more, when you list the history in Vo, it shows the offsets in the numeric form. Can you substitute them with flag names where appropriate?

@@ -325,6 +338,24 @@ static int r_line_hist_down() {
if (I.hist_down) {
return I.hist_down (I.user);
}
if (I.offset_prompt) {

This comment has been minimized.

@radare

radare Mar 8, 2018

Owner

we use tabs. not spaces

return false;
}
char line[R_LINE_BUFSIZE - 1];
sprintf(line, "0x%"PFMT64x, undo->seek[undo->idx + I.offset_index]);

This comment has been minimized.

@radare

radare Mar 8, 2018

Owner

use snprintf or r_str_newf and avoid abusing the stack with this char line[R_LINE_BUFSIZE] because to fill a PFMT64x you dont need more than 64 bytes. not 4096

if (I.offset_prompt) {
RCore *core = I.user;
RIOUndo *undo = &core->io->undo;
char line[R_LINE_BUFSIZE - 1];

This comment has been minimized.

@radare

radare Mar 8, 2018

Owner

dont do 4096 in the stack. you only need 64bytes to do an snprintf of pfmt64x

@@ -313,6 +313,19 @@ static int r_line_hist_up() {
if (!I.history.data) {
inithist ();
}
if (I.offset_prompt) {

This comment has been minimized.

@radare

radare Mar 8, 2018

Owner

use tabs

This comment has been minimized.

@radare

radare Mar 8, 2018

Owner

ideally the solution should be to delegate the logic of the history to the caller, not embed that in here

@@ -313,6 +313,19 @@ static int r_line_hist_up() {
if (!I.history.data) {
inithist ();
}
if (I.offset_prompt) {
RCore *core = I.user;
RIOUndo *undo = &core->io->undo;

This comment has been minimized.

@radare

radare Mar 8, 2018

Owner

rcons shouldnt have access to core or io

@@ -845,13 +845,16 @@ static void reset_print_cur(RPrint *p) {
static void visual_offset(RCore *core) {
char buf[256];
r_line_set_prompt ("[offset]> ");
core->cons->line->offset_prompt = true;
core->cons->line->offset_index = 0;

This comment has been minimized.

@radare

radare Mar 8, 2018

Owner

what about doing: r_line_set_history_callback() and implement the logic of offset_prompt in here?

@cyanpencil

This comment has been minimized.

Copy link
Contributor Author

cyanpencil commented Mar 8, 2018

Pancake, thank you for your useful reviews.

I fixed the whitespace and changed sprintf to r_str_newf as you suggested.
I'm looking right now at how to use r_line_set_history_callback() and removing RCore access from rcons, will update soon.

And one more, when you list the history in Vo, it shows the offsets in the numeric form. Can you substitute them with flag names where appropriate?

Yes, I have an idea on how to do that. I was thinking about using r_flag_get_at(offset)...it would be a clean and easy solution, but that would also mean adding r_flag as a dependency in libr/cons/Makefile (otherwise I get an undefined reference error).
Can I do that, or should I find another way? @XVilka

if (I.offset_prompt) {
RCore *core = I.user;
RIOUndo *undo = &core->io->undo;
/*char line[R_LINE_BUFSIZE - 1];*/

This comment has been minimized.

@XVilka

XVilka Mar 8, 2018

Collaborator

If it is not used - remove it.

RIOUndo *undo = &core->io->undo;
/*char line[R_LINE_BUFSIZE - 1];*/
if (I.offset_index <= -undo->undos) {
return false;

This comment has been minimized.

@XVilka

XVilka Mar 8, 2018

Collaborator

wrong indentation

return false;
}
I.offset_index--;
char *line = r_str_newf("0x%"PFMT64x, undo->seek[undo->idx + I.offset_index].off);

This comment has been minimized.

@XVilka

XVilka Mar 8, 2018

Collaborator

use space before (

RCore *core = I.user;
RIOUndo *undo = &core->io->undo;
if (I.offset_index >= undo->redos) {
return false;

This comment has been minimized.

@XVilka

XVilka Mar 8, 2018

Collaborator

wrong indentation

@cyanpencil

This comment has been minimized.

Copy link
Contributor Author

cyanpencil commented Mar 8, 2018

Sorry for the delay: I had more difficulty than expected with callbacks.
Now r_line_hist_up() calls the callback I.history_up_cb(), which can be either cmd_history_up() or offset_history_up(), depending if we are in offset prompt mode or in normal command line mode. Viceversa for r_line_hist_down().

I'm not totally sure about the positions I chose for those functions... (I chose to put cmd_history_up inside dietline.c and offset_history_up() in visual.c for clarity; but I had to add #include <r_cons.h> inside visual.c). Please tell me if there is a better way to organize this piece of code.

Also, I added the fact that if a given offset matches a flag, the flag name is displayed instead of the numeric offset.

I tried to remove RCore *rcore = line->user to avoid RCons accessing RCore (and io); but I have no ideas on how to retrieve the seek history without it (because it is stored in core->io->undo->seek[])...

In the final commit I also added the same functionality to offset prompt inside graph view.

Again, thanks for the reviews; they help me improve a lot.

@radare radare merged commit cd719d1 into radare:master Mar 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@radare

This comment has been minimized.

Copy link
Owner

radare commented Mar 9, 2018

@cyanpencil cyanpencil deleted the cyanpencil:offset-history branch Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.