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
Merged

Offset history #9591

merged 12 commits into from Mar 8, 2018

Conversation

@cyanpencil
Copy link
Contributor

@cyanpencil 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
Collaborator

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

This comment has been minimized.

@radare

radare Mar 5, 2018
Collaborator

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

@radare
Copy link
Collaborator

@radare 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
Copy link
Contributor Author

@cyanpencil 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
Copy link
Collaborator

@radare radare commented Mar 5, 2018

@XVilka
Copy link
Contributor

@XVilka 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
Copy link
Contributor

@Maijin Maijin commented Mar 6, 2018

Yeah Vo is nice

@XVilka
Copy link
Contributor

@XVilka 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
Copy link
Collaborator

@radare radare commented Mar 7, 2018

@cyanpencil
Copy link
Contributor Author

@cyanpencil 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
Copy link
Contributor Author

@cyanpencil 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
Copy link
Contributor

@XVilka 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
Copy link
Contributor

@XVilka 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
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

use tabs

This comment has been minimized.

@radare

radare Mar 8, 2018
Collaborator

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
Collaborator

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
Collaborator

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

@cyanpencil
Copy link
Contributor Author

@cyanpencil 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
Contributor

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
Contributor

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
Contributor

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
Contributor

wrong indentation

@cyanpencil
Copy link
Contributor Author

@cyanpencil 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 radareorg:master Mar 8, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@radare
Copy link
Collaborator

@radare radare commented Mar 9, 2018

@cyanpencil cyanpencil deleted the cyanpencil:offset-history branch Mar 9, 2018
SakiiR pushed a commit to SakiiR/radare2 that referenced this pull request Jul 1, 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants