Offset history #9591

cyanpencil · 2018-03-05T16:28:13Z

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...

radare · 2018-03-05T16:52:37Z

libr/cons/dietline.c

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


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

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

radare · 2018-03-05T16:56:48Z

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 · 2018-03-05T17:36:04Z

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 · 2018-03-05T21:49:31Z

On 5 Mar 2018, at 18:36, cyanpencil ***@***.***> wrote: 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-)

yep, sounds good to me

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.

+1

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9591 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3-lsrtApduOZgVR539JaxakzWEktrmks5tbXeGgaJpZM4SclVM>.

XVilka · 2018-03-06T17:11:21Z

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 · 2018-03-06T18:01:48Z

Yeah Vo is nice

XVilka · 2018-03-07T08:14:15Z

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 · 2018-03-07T09:58:07Z

@cyanpencil ping

cyanpencil · 2018-03-07T12:30:51Z

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 · 2018-03-08T00:32:57Z

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 · 2018-03-08T04:43:49Z

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 · 2018-03-08T04:46:01Z

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?

radare · 2018-03-08T08:53:50Z

libr/cons/dietline.c

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


we use tabs. not spaces

radare · 2018-03-08T08:55:36Z

libr/cons/dietline.c

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


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

radare · 2018-03-08T08:56:05Z

libr/cons/dietline.c

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


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

radare · 2018-03-08T08:56:11Z

libr/cons/dietline.c

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


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

radare · 2018-03-08T08:57:01Z

libr/cons/dietline.c

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


rcons shouldnt have access to core or io

radare · 2018-03-08T08:58:03Z

libr/core/visual.c

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


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

cyanpencil · 2018-03-08T10:47:23Z

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

XVilka · 2018-03-08T12:01:24Z

libr/cons/dietline.c

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


If it is not used - remove it.

XVilka · 2018-03-08T12:01:38Z

libr/cons/dietline.c

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


wrong indentation

XVilka · 2018-03-08T12:01:58Z

libr/cons/dietline.c

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


use space before (

XVilka · 2018-03-08T12:02:12Z

libr/cons/dietline.c

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


wrong indentation

cyanpencil · 2018-03-08T22:47:20Z

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 · 2018-03-09T07:59:25Z

🎉🎉🎉

…

On 8 Mar 2018, at 23:47, cyanpencil ***@***.***> wrote: 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. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

cyanpencil added 3 commits March 5, 2018 17:00

Fix offset history

23999ed

Refactor

349da7e

Refactor tabs

cc68dc4

radare reviewed Mar 5, 2018

View reviewed changes

Refactor strcmp

32f711c

cyanpencil added 3 commits March 7, 2018 15:35

boolean flag offset_prompt

511f834

offset autocomplete + offset history

0efed08

whitespace fix

7380086

fix warning

a6757c3

radare reviewed Mar 8, 2018

View reviewed changes

fix tabs + sprintf to r_str_newf

11c6840

XVilka reviewed Mar 8, 2018

View reviewed changes

cyanpencil added 3 commits March 8, 2018 22:41

added callbacks

3e98cda

flag names instead of numeric offsets

b0c593c

small fix + offset history in graph view

03b12d9

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

cyanpencil deleted the offset-history branch March 9, 2018 10:44

SakiiR pushed a commit to SakiiR/radare2 that referenced this pull request Jul 1, 2019

Implement pluggable dietline history. Use it in Vo (radareorg#9591)

dd94194

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offset history #9591

Offset history #9591

cyanpencil commented Mar 5, 2018

radare Mar 5, 2018

radare Mar 5, 2018

radare commented Mar 5, 2018

cyanpencil commented Mar 5, 2018 •

edited

radare commented Mar 5, 2018 via email

XVilka commented Mar 6, 2018

Maijin commented Mar 6, 2018

XVilka commented Mar 7, 2018

radare commented Mar 7, 2018

cyanpencil commented Mar 7, 2018

cyanpencil commented Mar 8, 2018

XVilka commented Mar 8, 2018

XVilka commented Mar 8, 2018

radare Mar 8, 2018

radare Mar 8, 2018

radare Mar 8, 2018

radare Mar 8, 2018

radare Mar 8, 2018

radare Mar 8, 2018

radare Mar 8, 2018

cyanpencil commented Mar 8, 2018

XVilka Mar 8, 2018

XVilka Mar 8, 2018

XVilka Mar 8, 2018

XVilka Mar 8, 2018

cyanpencil commented Mar 8, 2018

radare commented Mar 9, 2018 via email

Offset history #9591

Offset history #9591

Conversation

cyanpencil commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radare commented Mar 5, 2018

cyanpencil commented Mar 5, 2018 • edited

radare commented Mar 5, 2018 via email

XVilka commented Mar 6, 2018

Maijin commented Mar 6, 2018

XVilka commented Mar 7, 2018

radare commented Mar 7, 2018

cyanpencil commented Mar 7, 2018

cyanpencil commented Mar 8, 2018

XVilka commented Mar 8, 2018

XVilka commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanpencil commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyanpencil commented Mar 8, 2018

radare commented Mar 9, 2018 via email

cyanpencil commented Mar 5, 2018 •

edited