vimprobable-users

[Vimprobable-users] Editor patch evolution

[Markus D. <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?


Cheers,

      Markus

Re: [Vimprobable-users] Editor patch evolution

[Jason R. <jas...@gm...>]

On 28/09/12 at 07:06pm, Markus Demleitner wrote:
> 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.
> 
Once again, thanks Markus - works as advertised!

The greyed-out text box is a nice touch.

Cheers,

/J  

-- 

http://jasonwryan.com/  [GnuPG Key: B1BD4E40]

[Vimprobable-users] Editor patch evolution

[Daniel C. <dan...@gm...>]

Hi Markus!

This patch works fine for me too.
I'm not sure about the consequences, but I think it would be a better approach
to use webkit functions like webkit_dom_html_text_area_element_set_value()
instead of evaluating JavaScript. I suppose that using the c functions direct
could be a little faster.

I attached a patch to show what I mean. This is only a sample, but it should
be also possible to remove jsapi calls from _resume_from_editor() too.

Daniel

Re: [Vimprobable-users] Editor patch evolution

[Hannes S. <ha...@yl...>]

Hi!

Daniel Carl <dan...@gm...> wrote:
> I'm not sure about the consequences, but I think it would be a better
> approach to use webkit functions like
> webkit_dom_html_text_area_element_set_value() instead of evaluating
> JavaScript. I suppose that using the c functions direct could be a
> little faster.

This would imply a jump in the required Webkit version. Currently, the
browser still compiles fine witih 1.2.x.

Hannes

[Vimprobable-users] Editor patch evolution

[Daniel C. <dan...@gm...>]

Hannes Schüller <ha...@yl...> wrote:
> This would imply a jump in the required Webkit version. Currently, the
> browser still compiles fine witih 1.2.x.
> 
> Hannes

That's a reason. I thinks I should have a look in the right version of webkit
before hacking a patch:)

Daniel

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

Re: [Vimprobable-users] Editor patch evolution

[Markus D. <msd...@ar...>]

Hi Serge,

On Fri, Oct 05, 2012 at 02:48:50PM +0000, Serge E. Hallyn wrote:
> Quoting Markus Demleitner (msd...@ar...):
> > During a longish train ride today, I polished up the editor patch a
> > bit.  For one, the editor now runs asynchronously, so vimprobable
> 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.

Well, the canonical way to do this is to inspect the TMPDIR
environment variable.  I'd like that better than to introduce a
special setting in vimprobablerc.  The downside is that you'd have to
use a little wrapper script if you want application-specific TMPDIRs.

Would that work for you?

Cheers,

       Markus

Re: [Vimprobable-users] Editor patch evolution

[Alexander F. <ale...@gm...>]

Possibly one could introduce an option in vimprobablerc which
overrides the environment if it is specified? This seems to be common
in other UNIX applications.

Regards,
Alexander Foremny

2012/10/6 Markus Demleitner <msd...@ar...>:
> Hi Serge,
>
> On Fri, Oct 05, 2012 at 02:48:50PM +0000, Serge E. Hallyn wrote:
>> Quoting Markus Demleitner (msd...@ar...):
>> > During a longish train ride today, I polished up the editor patch a
>> > bit.  For one, the editor now runs asynchronously, so vimprobable
>> 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.
>
> Well, the canonical way to do this is to inspect the TMPDIR
> environment variable.  I'd like that better than to introduce a
> special setting in vimprobablerc.  The downside is that you'd have to
> use a little wrapper script if you want application-specific TMPDIRs.
>
> Would that work for you?
>
> Cheers,
>
>        Markus
>
>
> ------------------------------------------------------------------------------
> Don't let slow site performance ruin your business. Deploy New Relic APM
> Deploy New Relic app performance management and know exactly
> what is happening inside your Ruby, Python, PHP, Java, and .NET app
> Try New Relic at no cost today and get our sweet Data Nerd shirt too!
> http://p.sf.net/sfu/newrelic-dev2dev
> _______________________________________________
> Vimprobable-users mailing list
> Vim...@li...
> https://lists.sourceforge.net/lists/listinfo/vimprobable-users

Re: [Vimprobable-users] Editor patch evolution

[Markus D. <msd...@ar...>]

Hi list,

On Sat, Oct 06, 2012 at 12:07:08PM +0200, Alexander Foremny wrote:
> Possibly one could introduce an option in vimprobablerc which
> overrides the environment if it is specified? This seems to be common
> in other UNIX applications.
Well, I'm not sure if all that configurability is not a bit of an
overkill for something like that, but it's not much code, so here's a
patch that

(a) evaluates TMPDIR, if set, and
(b) lets you set a variable tempdir in .vimprobablerc (or
interactively)

The patch applies on top of last week's editor patch (talking about
which: If I'd like to get this stuff into the upstream git, what
would I do?)

Cheers,

         Markus

Re: [Vimprobable-users] Editor patch evolution

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

Quoting Markus Demleitner (msd...@ar...):
> Hi list,
> 
> On Sat, Oct 06, 2012 at 12:07:08PM +0200, Alexander Foremny wrote:
> > Possibly one could introduce an option in vimprobablerc which
> > overrides the environment if it is specified? This seems to be common
> > in other UNIX applications.
> Well, I'm not sure if all that configurability is not a bit of an
> overkill for something like that, but it's not much code, so here's a
> patch that
> 
> (a) evaluates TMPDIR, if set, and
> (b) lets you set a variable tempdir in .vimprobablerc (or
> interactively)

Thanks - this suffices for me.  I could see it being backward for some
people where inherited env gives them a TMPDIR, but mine is unset by
default.

Indeed since I run vimprobable through tabbed, setting a vimprobable-
specific TMPDIR would be more convoluted than setting it in the
vimprobablerc, so this is perfect.

> The patch applies on top of last week's editor patch (talking about
> which: If I'd like to get this stuff into the upstream git, what
> would I do?)
> 
> Cheers,
> 
>          Markus

> diff --git a/config.h b/config.h
> index 45957c4..860a0fc 100644
> --- a/config.h
> +++ b/config.h
> @@ -20,6 +20,7 @@ static const gboolean enablePlugins     = TRUE; /* TRUE keeps plugins enabled */
>  static const gboolean enableJava        = TRUE; /* FALSE disables Java applets */
>  static const gboolean enablePagecache   = FALSE; /* TRUE turns on the page cache. */
>  static gboolean escape_input_on_load    = TRUE; /* TRUE will disable automatic focusing of input fields via Javascript*/
> +char temp_dir[MAX_SETTING_SIZE]         = "/tmp"; /* location of temporary files, default will be overridden if TEMPDIR is set */
>  
>  /* appearance */
>  char statusbgcolor[MAX_SETTING_SIZE]    = "#000000";            /* background color for status bar */
> @@ -209,4 +210,5 @@ static Setting browsersettings[] = {
>      { "escapeinput",     NULL,               "",                           FALSE,          TRUE,            FALSE,          FALSE  },
>      { "strictssl",       NULL,               "",                            FALSE,          TRUE,            FALSE,          FALSE  },
>      { "cabundle",        ca_bundle,          "",                            FALSE,          FALSE,           FALSE,          FALSE  },
> +    { "tempdir",         temp_dir,           "",                            FALSE,          FALSE,           FALSE,          FALSE  },
>  };
> diff --git a/main.c b/main.c
> index 36e0edb..f82c2cb 100644
> --- a/main.c
> +++ b/main.c
> @@ -1837,7 +1837,8 @@ open_editor(const Arg *arg) {
>      gboolean success;
>      GPid child_pid;
>      gchar *value = NULL, *message = NULL, *tag = NULL, *edit_url = NULL;
> -    gchar *temp_file_name = g_strdup("/tmp/vimprobableeditXXXXXX");
> +    gchar *temp_file_name = g_strdup_printf("%s/vimprobableeditXXXXXX",
> +      temp_dir);
>      int temp_file_handle = -1;
>  
>      /* check if active element is suitable for text editing */
> @@ -2797,6 +2798,11 @@ main(int argc, char *argv[]) {
>          return EXIT_SUCCESS;
>      }
>  
> +    if (getenv("TMPDIR")) {
> +      strncpy(temp_dir, getenv("TMPDIR"), MAX_SETTING_SIZE);
> +      temp_dir[MAX_SETTING_SIZE-1] = 0;
> +    }
> +
>      if( getenv("XDG_CONFIG_HOME") )
>          config_base = g_strdup_printf("%s", getenv("XDG_CONFIG_HOME"));
>      else
> diff --git a/vimprobablerc.5 b/vimprobablerc.5
> index f69dda9..2964b20 100644
> --- a/vimprobablerc.5
> +++ b/vimprobablerc.5
> @@ -159,6 +159,9 @@ Reject or accept unverified certificates (default: true)
>  .IP cabundle=/path/to/file
>  Where CA certificates are stored (default: /etc/ssl/certs/ca-certificates.crt)
>  
> +.IP tempdir=/path/without/slash
> +A path to a directory for temporary files (default: $TMPDIR or /tmp)
> +
>  .SH MAPPINGS
>  
>  Keys can be mapped to the following functions:

> ------------------------------------------------------------------------------
> Don't let slow site performance ruin your business. Deploy New Relic APM
> Deploy New Relic app performance management and know exactly
> what is happening inside your Ruby, Python, PHP, Java, and .NET app
> Try New Relic at no cost today and get our sweet Data Nerd shirt too!
> http://p.sf.net/sfu/newrelic-dev2dev

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

Re: [Vimprobable-users] Editor patch evolution

[Hannes S. <ha...@yl...>]

Hello Markus!

Markus Demleitner <msd...@ar...> wrote:
> (talking about
> which: If I'd like to get this stuff into the upstream git, what
> would I do?)

As announced on IRC last week, I'm planning to merge the mature patches
(like yours) in a week or so and make a new release.

Hannes

Re: [Vimprobable-users] Editor patch evolution

[Markus D. <msd...@ar...>]

Hi,

On Sun, Oct 07, 2012 at 08:04:05PM +0200, Hannes Schüller wrote:
> Markus Demleitner <msd...@ar...> wrote:
> > (talking about
> > which: If I'd like to get this stuff into the upstream git, what
> > would I do?)
> 
> As announced on IRC last week, I'm planning to merge the mature patches
> (like yours) in a week or so and make a new release.

Ah -- are the requests in PATCHES still up-to-date, i.e., do you/does
anyone want individual patches for the commits on the list?  For the
external editor, that's five patches (the first one is a huge mess of
Hannes' initial changes and some additional hacking on it by yours
truly, but the others are, I think, fairly reasonable) -- or should I
push the stuff somewhere?  Would you, Hannes, rather pull from
somewhere?

Also, I've used the "goto error_exit" style in _resume_from_editor
since I had to heavily edit the code anyway.  Since I've not seen
that anywhere else in vimprobable, I thought I'd ask whether anyone
has large issues with this.  I happen to believe this is usually
preferable and more maintainable, but I'll change it to "local"
resource management if someone doesn't agree.

Then, there should be some documentation on this, viz,

* Key bindings: ^t opens an external editor
* use "handler vimprobableedit <your editor> %s" to configure which
  external editor you'd like to use
* the editor must open an X11 window and not fork from the parent;
  vim -gf does that, as does xterm -e vi

I'm not quite sure where to put the last two points; I'd appreciate
if someone else could add some appropriate prose to... erm...
whereever it belongs (the tempdir setting I've already mentioned in
vimprobablerc.5 -- is everyone fine with the name?).

Finally, should there be a default on the vimprobableedit handler
(vim -gf comes to mind)?  If so, what would be a good place to set
it?

Cheers,

         Markus

Re: [Vimprobable-users] Editor patch evolution

[Hannes S. <ha...@yl...>]

Hi!

Markus Demleitner <msd...@ar...> wrote:
> On Sun, Oct 07, 2012 at 08:04:05PM +0200, Hannes Schüller wrote:
> > Markus Demleitner <msd...@ar...> wrote:
> > > (talking about
> > > which: If I'd like to get this stuff into the upstream git, what
> > > would I do?)
> > 
> > As announced on IRC last week, I'm planning to merge the mature
> > patches (like yours) in a week or so and make a new release.
> 
> Ah -- are the requests in PATCHES still up-to-date, i.e., do you/does
> anyone want individual patches for the commits on the list?  For the
> external editor, that's five patches (the first one is a huge mess of
> Hannes' initial changes and some additional hacking on it by yours
> truly, but the others are, I think, fairly reasonable) -- or should I
> push the stuff somewhere?  Would you, Hannes, rather pull from
> somewhere?

I would certainly appreciate if you could wrap the final patches up in
a canonical way again and send them to the list. The main reason being
that the first patch in this thread (the one "based on HEAD") seems to
be reversed. That would save us some back and forth history.

Also, there seems to be a bit of a mixture between spaces and tabs
indentation in the patches.

> Also, I've used the "goto error_exit" style in _resume_from_editor
> since I had to heavily edit the code anyway.  Since I've not seen
> that anywhere else in vimprobable, I thought I'd ask whether anyone
> has large issues with this.  I happen to believe this is usually
> preferable and more maintainable, but I'll change it to "local"
> resource management if someone doesn't agree.

OK with me - anyone else?

> Then, there should be some documentation on this, viz,
> 
> * Key bindings: ^t opens an external editor
> * use "handler vimprobableedit <your editor> %s" to configure which
>   external editor you'd like to use
> * the editor must open an X11 window and not fork from the parent;
>   vim -gf does that, as does xterm -e vi
> 
> I'm not quite sure where to put the last two points;

The basic description should go in vimprobable2.1, I'd say, just like
the external URI handlers description. The handler setting (:set and rc
file) should go in vimprobablerc.5.

> Finally, should there be a default on the vimprobableedit handler
> (vim -gf comes to mind)?  If so, what would be a good place to set
> it?

I would think launching regular vim in a terminal editor would suit
most users better, but please set me right, folks!

Hannes

Re: [Vimprobable-users] Editor patch evolution

[Matthew C. <je...@gm...>]

Hi all,

I would prefer:

xterm -e vim

Over:

vim -gf

For the default.

Thanks,
-Matt

On Mon, Oct 08, 2012 at 11:18:17AM +0200, Hannes Schüller wrote:
> Hi!
> 
> Markus Demleitner <msd...@ar...> wrote:
> > On Sun, Oct 07, 2012 at 08:04:05PM +0200, Hannes Schüller wrote:
> > > Markus Demleitner <msd...@ar...> wrote:
> > > > (talking about
> > > > which: If I'd like to get this stuff into the upstream git, what
> > > > would I do?)
> > > 
> > > As announced on IRC last week, I'm planning to merge the mature
> > > patches (like yours) in a week or so and make a new release.
> > 
> > Ah -- are the requests in PATCHES still up-to-date, i.e., do you/does
> > anyone want individual patches for the commits on the list?  For the
> > external editor, that's five patches (the first one is a huge mess of
> > Hannes' initial changes and some additional hacking on it by yours
> > truly, but the others are, I think, fairly reasonable) -- or should I
> > push the stuff somewhere?  Would you, Hannes, rather pull from
> > somewhere?
> 
> I would certainly appreciate if you could wrap the final patches up in
> a canonical way again and send them to the list. The main reason being
> that the first patch in this thread (the one "based on HEAD") seems to
> be reversed. That would save us some back and forth history.
> 
> Also, there seems to be a bit of a mixture between spaces and tabs
> indentation in the patches.
> 
> > Also, I've used the "goto error_exit" style in _resume_from_editor
> > since I had to heavily edit the code anyway.  Since I've not seen
> > that anywhere else in vimprobable, I thought I'd ask whether anyone
> > has large issues with this.  I happen to believe this is usually
> > preferable and more maintainable, but I'll change it to "local"
> > resource management if someone doesn't agree.
> 
> OK with me - anyone else?
> 
> > Then, there should be some documentation on this, viz,
> > 
> > * Key bindings: ^t opens an external editor
> > * use "handler vimprobableedit <your editor> %s" to configure which
> >   external editor you'd like to use
> > * the editor must open an X11 window and not fork from the parent;
> >   vim -gf does that, as does xterm -e vi
> > 
> > I'm not quite sure where to put the last two points;
> 
> The basic description should go in vimprobable2.1, I'd say, just like
> the external URI handlers description. The handler setting (:set and rc
> file) should go in vimprobablerc.5.
> 
> > Finally, should there be a default on the vimprobableedit handler
> > (vim -gf comes to mind)?  If so, what would be a good place to set
> > it?
> 
> I would think launching regular vim in a terminal editor would suit
> most users better, but please set me right, folks!
> 
> Hannes



> ------------------------------------------------------------------------------
> Don't let slow site performance ruin your business. Deploy New Relic APM
> Deploy New Relic app performance management and know exactly
> what is happening inside your Ruby, Python, PHP, Java, and .NET app
> Try New Relic at no cost today and get our sweet Data Nerd shirt too!
> http://p.sf.net/sfu/newrelic-dev2dev

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


-- 
Matthew Carter
je...@gm...

Re: [Vimprobable-users] Editor patch evolution

[Hannes S. <ha...@yl...>]

Latest incarnation merged