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

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Wed, 3 Feb 2016 09:22:19 +0100

On Tue, Feb 2, 2016 at 11:18 PM, Marc André Tanner <mat_AT_brain-dump.org> wrote:
> On Mon, Feb 01, 2016 at 09:55:32PM +0100, Silvan Jegen wrote:
>> There are still a lot of rough edges. We don't change the jumplist for
>> example, which means that we won't be able to jump back to the old
>> file.
>
> The jumplist is almost inexistent anyway, for starters it is
> currently only local to the current file.

Ok, I can put that onto the TODO-later list then...


> A few remarks about the implementation below ...
>
>> + [VIS_ACTION_OPEN_FILE_UNDER_CURSOR] = {
>> + "open-file-under-cursor",
>> + "Open file under the cursor",
>> + call, { .f = vis_open_file }
>> + },
>> + [VIS_ACTION_OPEN_FILE_UNDER_CURSOR_NEW_WINDOW] = {
>> + "open-file-under-cursor-new_window",
>> + "Open file under the cursor in a new window",
>> + call, { .f = vis_open_file_new_window }
>> + },
>
> Instead of adding new functions to vis.[ch] and using indirection via
> "call", add one "open_file_under_cursor" function to main.c. This
> function should inspect its arg->b parameter and depending on that
> open the new file either in the current or a new window.

Will do.


>> +int is_filename_boundry(int c) {
>> + return isspace(c) || (c == ';' ||
>> + c == '"' || c == '\'' );
>
> Should be static, funny indentation, useless parenthesis?

I tried to use the same indentation as the one used with
"is_word_boundry" but it doesn't look like I succeeded. I'll remove
the parenthesis which I put there for aesthetic reasons...


>> +size_t text_filename_start_prev(Text *txt, size_t pos) {
>> + return text_customword_start_prev(txt, pos, is_filename_boundry);
>> +}
>> +
>> +size_t text_filename_end_next(Text *txt, size_t pos) {
>> + return text_customword_end_next(txt, pos, is_filename_boundry);
>> +}
>
> Instead of introducing these one should write a
>
> Filerange text_object_filename(Text *txt, size_t pos)
>
> which would also take care of any needed offsets. Also I'm not sure
> whether using the "customword" functions is a good idea due to their
> behavior if you are already at the start/end of the word.

Yes, their behavior surprised me. I will look into implementing that function.


> Maybe the actual file opening should be implemented using the :edit
> command via vis_cmd(...) this would also take care about detecting
> unsaved changes etc.

That's what I wanted to do at first but then I saw that
"info_unsaved_changes" would show "... (add ! to override)" which does
not make sense. I will think about how to deal with that.


Cheers,

Silvan


> Marc André Tanner >< http://www.brain-dump.org/ >< GPG key: 10C93617
>
Received on Wed Feb 03 2016 - 09:22:19 CET

This archive was generated by hypermail 2.3.0 : Wed Feb 03 2016 - 09:24:12 CET