Re: [hackers][vis][PATCH] Implement a first version of the 'gf' family of commands

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Mon, 8 Feb 2016 11:02:03 +0100

Hi Marc

On Sun, Feb 7, 2016 at 4:37 PM, Marc André Tanner <mat_AT_brain-dump.org> wrote:
> The attached patches on top of current git HEAD should provide the same
> functionality you provided.
>
> Not sure whether they should be merged. Could be useful when displaying
> grep search results in a window as in:
>
> :| grep something *.c

I assume you mean that you're not sure if this functionality should go
in at all?

I find the feature really useful. If you edit config files it's nice
to jump directly to the configured file name (even if only to see
whether it exists at all). The functionality let's you jump easily to
files mentioned in the output of other programs. You mention grep but
there are others too ("make" output for example though vim has a
tighter integration there).

Some comments below.

> Date: Sun, 7 Feb 2016 16:06:31 +0100
> Subject: [PATCH 1/2] vis: export vis_window_closable
>
> ---
> vis-cmds.c | 10 ++--------
> vis.c | 10 ++++++++++
> vis.h | 3 +++
> 3 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/vis-cmds.c b/vis-cmds.c
> index 3f612f4..b3dad5b 100644
> --- a/vis-cmds.c
> +++ b/vis-cmds.c
> _AT_@ -429,19 +429,13 @@ static bool cmd_open(Vis *vis, Filerange *range, enum CmdOpt opt, const char *ar
> return openfiles(vis, &argv[1]);
> }
> [...]
>
> +Win *vis_window(Vis *vis) {
> + return vis->win;
> +}
> +
> Text *vis_file_text(File *file) {
> return file->text;
> }
> diff --git a/vis.h b/vis.h
> index 3bcd9c7..eb84695 100644
> --- a/vis.h
> +++ b/vis.h
> _AT_@ -67,6 +67,8 @@ void vis_suspend(Vis*);
> bool vis_window_new(Vis*, const char *filename);
> /* reload the file currently displayed in the window from disk */
> bool vis_window_reload(Win*);
> +/* check whether closing the window would loose unsaved changes */
> +bool vis_window_closable(Win*);
> /* close window, redraw user interface */
> void vis_window_close(Win*);
> /* split the given window. changes to the displayed text will be reflected
> _AT_@ -405,6 +407,7 @@ bool vis_signal_handler(Vis*, int signum, const siginfo_t *siginfo, const void *
> /* TODO: expose proper API to iterate through files etc */
> Text *vis_text(Vis*);
> View *vis_view(Vis*);
> +Win *vis_window(Vis*);
> Text *vis_file_text(File*);
> const char *vis_file_name(File*);
>
> --
> 2.1.4

LGTM. That's about how my second version looks as well. It may be
better to put the "vis_window" in your second patch because it's not used
in this one and the commit does not mention it.


> From b02aff69f90ffb5fe214d10e11386cadab2df55c Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marc=20Andr=C3=A9=20Tanner?= <mat_AT_brain-dump.org>
> Date: Sun, 7 Feb 2016 16:07:44 +0100
> Subject: [PATCH 2/2] vis: implement gf and <C-w>gf to open filename under
> cursor
>
> Based on a patch by Silvan Jegen.
> ---
> config.def.h | 2 ++
> main.c | 40 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/config.def.h b/config.def.h
> index 47ed326..774871b 100644
> --- a/config.def.h
> +++ b/config.def.h
> _AT_@ -233,6 +233,8 @@ static const KeyBinding bindings_normal[] = {
> { "gP", ACTION(PUT_BEFORE_END) },
> { "~", ALIAS("<vis-operator-case-swap>l") },
> { "<End>", ALIAS("$") },
> + { "gf", ACTION(OPEN_FILE_UNDER_CURSOR) },
> + { "<C-w>gf", ACTION(OPEN_FILE_UNDER_CURSOR_NEW_WINDOW) },
> { 0 /* empty last element, array terminator */ },
> };
>
> diff --git a/main.c b/main.c
> index c3ae7a3..21a1066 100644
> --- a/main.c
> +++ b/main.c
> _AT_@ -111,6 +111,8 @@ static const char *unicode_info(Vis*, const char *keys, const Arg *arg);
> static const char *percent(Vis*, const char *keys, const Arg *arg);
> /* either increment (arg->i > 0) or decrement (arg->i < 0) number under cursor */
> static const char *number_increment_decrement(Vis*, const char *keys, const Arg *arg);
> +/* open a filename under cursor in same (!arg->b) or new (arg->b) window */
> +static const char *open_file_under_cursor(Vis*, const char *keys, const Arg *arg);
>
> enum {
> VIS_ACTION_EDITOR_SUSPEND,
> _AT_@ -272,6 +274,8 @@ enum {
> VIS_ACTION_UNICODE_INFO,
> VIS_ACTION_NUMBER_INCREMENT,
> VIS_ACTION_NUMBER_DECREMENT,
> + VIS_ACTION_OPEN_FILE_UNDER_CURSOR,
> + VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW,
> VIS_ACTION_NOP,
> };
>
> _AT_@ -1071,6 +1075,16 @@ static KeyAction vis_action[] = {
> "Decrement number under cursor",
> number_increment_decrement, { .i = -1 }
> },
> + [VIS_ACTION_OPEN_FILE_UNDER_CURSOR] = {
> + "open-file-under-cursor",
> + "Open file under the cursor",
> + open_file_under_cursor, { .b = false }
> + },
> + [VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW] = {
> + "open-file-under-cursor-new-cursor",
> + "Open file under the cursor in a new window",
> + open_file_under_cursor, { .b = true }
> + },
> [VIS_ACTION_NOP] = {
> "nop",
> "Ignore key, do nothing",
> _AT_@ -1677,6 +1691,32 @@ static const char *number_increment_decrement(Vis *vis, const char *keys, const
> return keys;
> }
>
> +static const char *open_file_under_cursor(Vis *vis, const char *keys, const Arg *arg) {
> + Win *win = vis_window(vis);
> + View *view = vis_view(vis);
> + Text *txt = vis_text(vis);
> +
> + if (!arg->b && !vis_window_closable(win)) {
> + vis_info_show(vis, "No write since last change");
> + return keys;
> + }
> +
> + for (Cursor *c = view_cursors(view); c; c = view_cursors_next(c)) {

In my version I did not take care of multiple cursors because I have
only a vague understanding of the concept.


> + Filerange r = text_object_filename(txt, view_cursors_pos(c));
> + if (!text_range_valid(&r))
> + continue;
> + char *name = text_bytes_alloc0(txt, r.start, text_range_size(&r));
> + if (name && vis_window_new(vis, name) && !arg->b) {

The reason I have not yet sent my version of the patch is that I am not
sure what to do when we open a file like

"nonexistentdir/test.txt"

At the moment we just open the new file but cannot save it. vis shows
an error like "file cannot be saved" upon :w. To be able to save it you
currently just have to change the file name of course.

I am not sure if it would be better to implement some functions that check
for the validity of the file name given and return a more helpful error
like "file path does not exist".

What do you think? If you think that's not needed I would vote for your
patch going in as is.


Cheers,

Silvan


> + vis_window_close(win);
> + free(name);
> + return keys;
> + }
> + free(name);
> + }
> +
> + return keys;
> +}
> +
> static Vis *vis;
>
> static void signal_handler(int signum, siginfo_t *siginfo, void *context) {
> --
> 2.1.4
Received on Mon Feb 08 2016 - 11:02:03 CET