Re: [Vimprobable-users] Editor patch evolution

[Serge E. H. <se...@ha...>]

Quoting Markus Demleitner (msd...@ar...):
> Hi,
> 
> During a longish train ride today, I polished up the editor patch a
> bit.  For one, the editor now runs asynchronously, so vimprobable
> doesn't just sit there as long as the editor runs.  To keep people
> from futzing around in the original text field, it is now grayed out
> while the editor is active.
> 
> I've also fixed the editing behaviour in the presence of " characters
> in the content.  I'm not quite happy with this, though -- javascript
> escaping must be more complex than this.  Can anyone point to a
> comprehensive and authoritative answer as to how javascript expects
> its string liteals?
> 
> Attached are two patches; one should apply on top of the patch
> yesterday, the other should apply on a clean 1.1.1 release.
> 
> Any feedback is welcome.  Is there anyone I should my git
> format-patch to assuming people like this?

Hi,

Very nice, thanks!

Do you mind making the directory for the tempfiles configurable?  I'm
currently hardcoding it to getenv("HOME")/Downloads rather than using /tmp,
because that is the only directory to which I allow vimprobable and its
children write access.  It'd be nice if it could simply be set in
vimprobablerc.

> Cheers,
> 
>       Markus

> diff --git a/Makefile b/Makefile
> index 7493657..772a30c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -17,7 +17,7 @@ INSTALL = $(BINDIR)/$(TARGET) $(MANINSTALL)
>  # DEBUG build?  Off by default
>  V_DEBUG = 0
>  
> -CFLAGS += `pkg-config --cflags $(LIBS)` -D_XOPEN_SOURCE=500
> +CFLAGS += `pkg-config --cflags $(LIBS)`
>  LDFLAGS += `pkg-config --libs $(LIBS)` -lX11 -lXext
>  
>  # TA:  This is a pretty stringent list of warnings to bail on!
> diff --git a/keymap.h b/keymap.h
> index 86aea55..7ba5d0b 100644
> --- a/keymap.h
> +++ b/keymap.h
> @@ -114,9 +114,6 @@ Key keys[] = {
>      { 0,                    GDK_semicolon,  GDK_T,          input,      {.s = ";T"} },
>      { 0,                    GDK_semicolon,  GDK_W,          input,      {.s = ";W"} },
>  
> -    /* this needs to be a binding using CTRL for obvious reasons */
> -    { GDK_CONTROL_MASK,     0,              GDK_t,          open_editor,{} },
> -
>      { 0,                    GDK_VoidSymbol, GDK_Escape,     set,        {ModeNormal} },
>      { GDK_CONTROL_MASK,     GDK_VoidSymbol, GDK_bracketleft,set,        {ModeNormal} },
>      { GDK_CONTROL_MASK,     0,              GDK_z,          set,        {ModePassThrough} },
> diff --git a/main.c b/main.c
> index 36e0edb..b3407ba 100644
> --- a/main.c
> +++ b/main.c
> @@ -11,10 +11,7 @@
>  */
>  
>  #include <X11/Xlib.h>
> -#include <sys/types.h>
> -#include <sys/wait.h>
>  #include <errno.h>
> -#include <stdlib.h>
>  #include "includes.h"
>  #include "vimprobable.h"
>  #include "utilities.h"
> @@ -64,8 +61,6 @@ static gboolean complete(const Arg *arg);
>  static gboolean descend(const Arg *arg);
>  gboolean echo(const Arg *arg);
>  static gboolean focus_input(const Arg *arg);
> -static gboolean open_editor(const Arg *arg);
> -void _resume_from_editor(GPid child_pid, int status, gpointer data);
>  static gboolean input(const Arg *arg);
>  static gboolean navigate(const Arg *arg);
>  static gboolean number(const Arg *arg);
> @@ -437,9 +432,6 @@ webview_keypress_cb(WebKitWebView *webview, GdkEventKey *event) {
>              g_free(a.s);
>              a.i = ModeNormal;
>              return set(&a);
> -        } else if (CLEAN(event->state) & GDK_CONTROL_MASK) {
> -            /* keybindings of non-printable characters */
> -            if (process_keypress(event) == TRUE) return TRUE;
>          }
>      case ModePassThrough:
>          if (IS_ESCAPE(event)) {
> @@ -1829,182 +1821,6 @@ view_source(const Arg * arg) {
>      return TRUE;
>  }
>  
> -/* open an external editor defined by the protocol handler for
> -vimprobableedit on a text box or similar */
> -static gboolean
> -open_editor(const Arg *arg) {
> -    char *text = NULL;
> -    gboolean success;
> -    GPid child_pid;
> -    gchar *value = NULL, *message = NULL, *tag = NULL, *edit_url = NULL;
> -    gchar *temp_file_name = g_strdup("/tmp/vimprobableeditXXXXXX");
> -    int temp_file_handle = -1;
> -
> -    /* check if active element is suitable for text editing */
> -    jsapi_evaluate_script("document.activeElement.tagName", &value, &message);
> -    if (value == NULL)
> -        return FALSE;
> -    tag = g_strdup(value);
> -    if (strcmp(tag, "INPUT") == 0) {
> -        /* extra check: type == text */
> -        jsapi_evaluate_script("document.activeElement.type", &value, &message);
> -        if (strcmp(value, "text") != 0) {
> -            g_free(value);
> -            g_free(message);
> -            return FALSE;
> -        }
> -    } else if (strcmp(tag, "TEXTAREA") != 0) {
> -        g_free(value);
> -        g_free(message);
> -        return FALSE;
> -    }
> -    jsapi_evaluate_script("document.activeElement.value", &value, &message);
> -    text = g_strdup(value);
> -    if (text == NULL) {
> -        g_free(value);
> -        g_free(message);
> -        return FALSE;
> -    }
> -
> -    /* write text into temporary file */
> -    temp_file_handle = mkstemp(temp_file_name);
> -    if (temp_file_handle == -1) {
> -        message = g_strdup_printf("Could not create temporary file: %s",
> -            strerror(errno));
> -        give_feedback(message);
> -        g_free(value);
> -        g_free(message);
> -        g_free(text);
> -        return FALSE;
> -    }
> -    if (write(temp_file_handle, text, strlen(text)) != strlen(text)) {
> -        message = g_strdup_printf("Short write to temporary file: %s",
> -            strerror(errno));
> -        give_feedback(message);
> -        g_free(value);
> -        g_free(message);
> -        g_free(text);
> -        return FALSE;
> -	}
> -    close(temp_file_handle);
> -    g_free(text);
> -
> -    /* spawn editor */
> -    edit_url = g_strdup_printf("vimprobableedit:%s", temp_file_name);
> -    success = open_handler_pid(edit_url, &child_pid);
> -    g_free(edit_url);
> -    if (!success) {
> -        give_feedback("External editor open failed (no handler for"
> -            " vimprobableedit protocol?)");
> -        unlink(temp_file_name);
> -        g_free(value);
> -        g_free(message);
> -        return FALSE;
> -    }
> -   
> -    /* mark the active text box as "under processing" */
> -    jsapi_evaluate_script(
> -        "document.activeElement.disabled = true;"
> -        "document.activeElement.originalBackground = "
> -        "   document.activeElement.style.background;"
> -        "document.activeElement.style.background = '#aaaaaa';"
> -        ,&value, &message);
> -
> -    g_child_watch_add(child_pid, _resume_from_editor, temp_file_name);
> -
> -    /* temp_file_name is freed in _resume_from_editor */
> -    g_free(value);
> -    g_free(message);
> -    g_free(tag);
> -    return TRUE;
> -}
> -
> -
> -/* pick up from where open_editor left the work to the glib event loop.
> -
> -This is called when the external editor exits. 
> -
> -The data argument points to allocated memory containing the temporary file
> -name. */
> -void 
> -_resume_from_editor(GPid child_pid, int child_status, gpointer data) {
> -    FILE *fp;
> -    GString *set_value_js = g_string_new(
> -        "document.activeElement.value = \"");
> -    g_spawn_close_pid(child_pid);
> -    gchar *value = NULL, *message = NULL;
> -    gchar *temp_file_name = data;
> -    gchar buffer[BUF_SIZE] = ""; 
> -    gchar *buf_ptr = buffer;
> -    int char_read;
> -
> -    jsapi_evaluate_script(
> -        "document.activeElement.disabled = true;"
> -        "document.activeElement.style.background = '#aaaaaa';"
> -        ,&value, &message);
> -
> -    if (child_status) {
> -        give_feedback("External editor returned with non-zero status,"
> -            " discarding edits.");
> -        goto error_exit;
> -    }
> -
> -    /* re-read the new contents of the file and put it into the HTML element */
> -    if (!access(temp_file_name, R_OK) == 0) {
> -        message = g_strdup_printf("Could not access temporary file: %s",
> -            strerror(errno));
> -        goto error_exit;
> -    }
> -    fp = fopen(temp_file_name, "r");
> -    if (fp == NULL) {
> -        /* this would be too weird to even emit an error message */
> -        goto error_exit;
> -    }
> -    jsapi_evaluate_script("document.activeElement.value = '';", 
> -        &value, &message);
> -
> -    while (EOF != (char_read = fgetc(fp))) {
> -        if (char_read == '\n') {
> -            *buf_ptr++ = '\\';
> -            *buf_ptr++ = 'n';
> -        } else if (char_read == '"') {
> -            *buf_ptr++ = '\\';
> -            *buf_ptr++ = '"';
> -        } else {
> -            *buf_ptr++ = char_read;
> -        }
> -        /* ship out as the buffer when space gets tight.  This has
> -        fuzz to save on thinking, plus we have enough space for the
> -        trailing "; in any case. */
> -        if (buf_ptr-buffer>=BUF_SIZE-10) {
> -            *buf_ptr = 0;
> -            g_string_append(set_value_js, buffer);
> -            buf_ptr = buffer;
> -        }
> -    }
> -    *buf_ptr++ = '"';
> -    *buf_ptr++ = ';';
> -    *buf_ptr = 0;
> -    g_string_append(set_value_js, buffer);
> -    fclose(fp);
> -
> -    jsapi_evaluate_script(set_value_js->str, &value, &message);
> -
> -    /* Fall through, error and normal exit are identical */
> -error_exit:
> -    jsapi_evaluate_script(
> -        "document.activeElement.disabled = false;"
> -        "document.activeElement.style.background ="
> -        "   document.activeElement.originalBackground;"
> -        ,&value, &message);
> -
> -    g_string_free(set_value_js, TRUE);
> -    unlink(temp_file_name);
> -    g_free(temp_file_name);
> -    g_free(value);
> -    g_free(message);
> -}
> -
>  static gboolean
>  focus_input(const Arg *arg) {
>      static Arg a;
> diff --git a/utilities.c b/utilities.c
> index fcef30b..6ee63d1 100644
> --- a/utilities.c
> +++ b/utilities.c
> @@ -795,22 +795,13 @@ void make_uri_handlers_list(URIHandler *uri_handlers, int length)
>      }
>  }
>  
> -
> -/* spawn a child process handling a protocol encoded in URI.
> -
> -On success, pid will contain the pid of the spawned child.
> -If you pass NULL as child_pid, glib will reap the child. */
>  gboolean
> -open_handler_pid(char *uri, GPid *child_pid) {
> +open_handler(char *uri) {
>      char *argv[64];
>      char *p = NULL, *arg, arg_temp[MAX_SETTING_SIZE], *temp, temp2[MAX_SETTING_SIZE] = "", *temp3;
>      int j;
>      GList *l;
> -    GSpawnFlags flags = G_SPAWN_SEARCH_PATH; 
>  
> -    if (child_pid) {
> -        flags |= G_SPAWN_DO_NOT_REAP_CHILD;
> -    }
>      p = strchr(uri, ':');
>      if (p) {
>      	if (dynamic_uri_handlers != NULL) {
> @@ -830,7 +821,7 @@ open_handler_pid(char *uri, GPid *child_pid) {
>                                  	strncat(arg_temp, temp3, 1);
>                                  	temp3++;
>                              	}
> -                            	strcat(arg_temp, arg+1);
> +                            	strcat(arg_temp, arg);
>                              	temp3++;
>                              	temp3++;
>                              	strcat(arg_temp, temp3);
> @@ -842,8 +833,7 @@ open_handler_pid(char *uri, GPid *child_pid) {
>                          	j++;
>                      	}
>                      	argv[j] =  NULL;
> -                    	g_spawn_async(NULL, argv, NULL, flags,
> -                    	    NULL, NULL, child_pid, NULL);
> +                    	g_spawn_async(NULL, argv, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL, NULL, NULL);
>                  	}
>                  	return TRUE;
>              	}
> @@ -853,8 +843,3 @@ open_handler_pid(char *uri, GPid *child_pid) {
>      return FALSE;
>  }
>  
> -
> -gboolean
> -open_handler(char *uri) {
> -	return open_handler_pid(uri, NULL);
> -}
> diff --git a/utilities.h b/utilities.h
> index 3b532d2..f9ac1ba 100644
> --- a/utilities.h
> +++ b/utilities.h
> @@ -35,4 +35,4 @@ char *find_uri_for_searchengine(const char *handle);
>  void make_searchengines_list(Searchengine *searchengines, int length);
>  void make_uri_handlers_list(URIHandler *uri_handlers, int length);
>  gboolean open_handler(char *uri);
> -gboolean open_handler_pid(char *uri, GPid *child_pid);
> +
> diff --git a/vimprobable.h b/vimprobable.h
> index f5d3e64..5a6c2df 100644
> --- a/vimprobable.h
> +++ b/vimprobable.h
> @@ -181,9 +181,6 @@ enum ConfigFileError {
>  #define             HISTORY_STORAGE_FILENAME    "%s/vimprobable/history", config_base
>  #define             CLOSED_URL_FILENAME         "%s/vimprobable/closed", config_base
>  
> -/* external editor - temporary file */
> -#define             TEMPFILE                    "/tmp/vimprobable_edit"
> -
>  /* Command size */
>  #define	            COMMANDSIZE	                1024
>  
> @@ -194,6 +191,3 @@ enum ConfigFileError {
>  
>  /* completion list size */
>  #define             MAX_LIST_SIZE               40
> -
> -/* Size of (some) I/O buffers */
> -#define             BUF_SIZE                    1024

> diff --git a/Makefile b/Makefile
> index fe39667..7493657 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -15,7 +15,7 @@ MANINSTALL = $(addprefix $(MANDIR)/man1/,$(MAN1)) \
>  INSTALL = $(BINDIR)/$(TARGET) $(MANINSTALL)
>  
>  # DEBUG build?  Off by default
> -V_DEBUG = 1
> +V_DEBUG = 0
>  
>  CFLAGS += `pkg-config --cflags $(LIBS)` -D_XOPEN_SOURCE=500
>  LDFLAGS += `pkg-config --libs $(LIBS)` -lX11 -lXext
> diff --git a/main.c b/main.c
> index a35e638..5acf509 100644
> --- a/main.c
> +++ b/main.c
> @@ -65,6 +65,7 @@ static gboolean descend(const Arg *arg);
>  gboolean echo(const Arg *arg);
>  static gboolean focus_input(const Arg *arg);
>  static gboolean open_editor(const Arg *arg);
> +void _resume_from_editor(GPid child_pid, int status, gpointer data);
>  static gboolean input(const Arg *arg);
>  static gboolean navigate(const Arg *arg);
>  static gboolean number(const Arg *arg);
> @@ -1832,14 +1833,11 @@ view_source(const Arg * arg) {
>  vimprobableedit on a text box or similar */
>  static gboolean
>  open_editor(const Arg *arg) {
> -    FILE *fp;
> -    char *text = NULL, s[255] = "";
> +    char *text = NULL;
>      gboolean success;
> -    GString *new_text = g_string_new("");
>      GPid child_pid;
> -    int child_status;
>      gchar *value = NULL, *message = NULL, *tag = NULL, *edit_url = NULL;
> -    gchar temp_file_name[] = "/tmp/vimprobableeditXXXXXX";
> +    gchar *temp_file_name = g_strdup("/tmp/vimprobableeditXXXXXX");
>      int temp_file_handle = -1;
>  
>      /* check if active element is suitable for text editing */
> @@ -1888,7 +1886,7 @@ open_editor(const Arg *arg) {
>          g_free(text);
>          return FALSE;
>  	}
> -	close(temp_file_handle);
> +    close(temp_file_handle);
>      g_free(text);
>  
>      /* spawn editor */
> @@ -1903,59 +1901,97 @@ open_editor(const Arg *arg) {
>          g_free(message);
>          return FALSE;
>      }
> -    
> -    /* Wait for the child to exit */
> -    /* TODO: use g_child_watch_add and make the rest a callback */
> -    while (waitpid(child_pid, &child_status, 0)) {
> -        if (errno!=EINTR) {
> -            break;
> -        }
> -    }
> -    g_spawn_close_pid (child_pid);
> +   
> +    /* mark the active text box as "under processing" */
> +    jsapi_evaluate_script(
> +        "document.activeElement.disabled = true;"
> +        "document.activeElement.originalBackground = "
> +        "   document.activeElement.style.background;"
> +        "document.activeElement.style.background = '#aaaaaa';"
> +        ,&value, &message);
> +
> +    g_child_watch_add(child_pid, _resume_from_editor, temp_file_name);
> +
> +    /* temp_file_name is freed in _resume_from_editor */
> +    g_free(value);
> +    g_free(message);
> +    g_free(tag);
> +    return TRUE;
> +}
> +
> +
> +/* pick up from where open_editor left the work to the glib event loop.
> +
> +This is called when the external editor exits. 
> +
> +The data argument points to allocated memory containing the temporary file
> +name. */
> +void 
> +_resume_from_editor(GPid child_pid, int child_status, gpointer data) {
> +    FILE *fp;
> +    GString *new_text = g_string_new("");
> +    g_spawn_close_pid(child_pid);
> +    gchar *value = NULL, *message = NULL;
> +    gchar *temp_file_name = data;
> +    gchar buffer[255] = "";
> +
> +    jsapi_evaluate_script(
> +        "document.activeElement.disabled = true;"
> +        "document.activeElement.style.background = '#aaaaaa';"
> +        ,&value, &message);
>  
>      if (child_status) {
>          give_feedback("External editor returned with non-zero status,"
>              " discarding edits.");
> -        unlink(temp_file_name);
> -        g_free(value);
> -        g_free(message);
> -        return FALSE;
> +        goto error_exit;
>      }
>  
>      /* re-read the new contents of the file and put it into the HTML element */
>      if (!access(temp_file_name, R_OK) == 0) {
> -        g_free(value);
> -        g_free(message);
> -        return FALSE;
> +        message = g_strdup_printf("Could not access temporary file: %s",
> +            strerror(errno));
> +        goto error_exit;
>      }
>      fp = fopen(temp_file_name, "r");
>      if (fp == NULL) {
> -        g_free(value);
> -        g_free(message);
> -        return FALSE;
> +        /* this would be too weird to even emit an error message */
> +        goto error_exit;
>      }
> -    jsapi_evaluate_script("document.activeElement.value = '';", &value, &message);
> +    jsapi_evaluate_script("document.activeElement.value = '';", 
> +        &value, &message);
>      new_text = g_string_append(new_text, "\"");
> -    while (fgets(s, 254, fp)) {
> -        if (s[strlen(s)-1] == '\n') {
> +    while (fgets(buffer, 254, fp)) {
> +        if (buffer[strlen(buffer)-1] == '\n') {
>              /* encode line breaks into the string as Javascript does not like actual line breaks */
> -            new_text = g_string_append_len(new_text, s, strlen(s) - 1);
> +            new_text = g_string_append_len(
> +                new_text, buffer, strlen(buffer) - 1);
>              new_text = g_string_append(new_text, "\\n");
>          } else {
> -            new_text = g_string_append(new_text, s);
> +            new_text = g_string_append(new_text, buffer);
>          }
>      }
>      new_text = g_string_append(new_text, "\"");
>      fclose(fp);
> -    jsapi_evaluate_script(g_strconcat("document.activeElement.value = ", new_text->str, ";", NULL), &value, &message);
> +    /* FIXME: Is the memory returned by g_strconcat actually freed? */
> +    jsapi_evaluate_script(g_strconcat("document.activeElement.value = ", 
> +        new_text->str, ";", NULL), &value, &message);
> +
> +    /* Fall through, error and normal exit are identical */
> +error_exit:
> +    if (new_text) {
> +        g_string_free(new_text, TRUE);
> +    }
>  
> -    /* done */
> -    g_string_free(new_text, TRUE);
> +    jsapi_evaluate_script(
> +        "document.activeElement.disabled = false;"
> +        "document.activeElement.style.background ="
> +        "   document.activeElement.originalBackground;"
> +        ,&value, &message);
> +
> +    unlink(temp_file_name);
> +    g_free(temp_file_name);
>      g_free(value);
>      g_free(message);
> -    g_free(tag);
> -    unlink(temp_file_name);
> -    return TRUE;
>  }
>  
>  static gboolean

> ------------------------------------------------------------------------------
> Got visibility?
> Most devs has no idea what their production app looks like.
> Find out how fast your code is with AppDynamics Lite.
> http://ad.doubleclick.net/clk;262219671;13503038;y?
> http://info.appdynamics.com/FreeJavaPerformanceDownload.html

> _______________________________________________
> Vimprobable-users mailing list
> Vim...@li...
> https://lists.sourceforge.net/lists/listinfo/vimprobable-users