bugGNU Octave - Bugs: bug #40600, edit: changing EDITOR after first...

 
 

bug #40600: edit: changing EDITOR after first call to edit() has no effect

Submitter:  Carnë Draug <carandraug>
Submitted:  Sat 16 Nov 2013 05:37:59 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 24 Nov 2013 03:31:28 AM UTC, comment #14: 

ok. I pushed that change. However, as discussed on IRC, this means dropping support for fancy things with the value of editor (such as gnudoit -q "(find-file \"%s\")" ).

For reference, in case someone wants to work on it, I have attached an alternative patch that makes things more complicated but should allow for all cases.

(file #29696)

Carnë Draug <carandraug>
Group Member
Sun 24 Nov 2013 03:14:27 AM UTC, comment #13: 

Sorry I was looking at the patch wrong, it does remove the assignment that used to be there. This change looks good to me.

Mike Miller <mtmiller>
Group Member
Sun 24 Nov 2013 12:57:48 AM UTC, comment #12: 

You still have a line in this patch that sets FUNCTION.EDITOR to the argument passed in to "edit editor", but that is thrown away on the next call to edit, right? Other than that I'm ok with this change.

Mike Miller <mtmiller>
Group Member
Sun 24 Nov 2013 12:42:26 AM UTC, comment #11: 

There's nothing special about this patch (see attached). It simply removes the editor option from edit, and makes it always check the value of EDITOR. I do not deprecate the option because EDITOR doesn not allow for some much flexibility.

However, this means that we can't have "fancy" things as editor such as "gnudoit -q \"(find-file \\\"%s\\\")\"". If you think this a useful thing, an alternative option (I'm inclined to this), would be to have EDITOR check if the new string has a %s. If it does, does nothing to it. Otherwise, it's EDITOR that appends %s to it.

(file #29694)

Carnë Draug <carandraug>
Group Member
Sat 23 Nov 2013 11:44:25 PM UTC, comment #10: 

Okay, when the path is applied I will update my patch #8240 in order to synchronize EDITOR with the GUI configuration.

Torsten Lilge <ttl>
Group Member
Sat 23 Nov 2013 11:34:06 PM UTC, comment #9: 

After talking with Carnë on #octave I'm coming around to having EDITOR be the one editor and deprecating the "edit EDITOR" setting. He has a patch in progress that seems to work to me.

Mike Miller <mtmiller>
Group Member
Sat 23 Nov 2013 11:10:37 PM UTC, comment #8: 

I disagree on making things clear on the documentation. Having a default text editor is such a simple thing that should not require documentation at all.

There should be one, and only one, default text editor that is used by Octave. And changing that value should change it for all of Octave.

Why would different functions require different text editors? The edit function already has the mode to specify async/sync mode.

Carnë Draug <carandraug>
Group Member
Sat 23 Nov 2013 11:00:16 PM UTC, comment #7: 

EDITOR () is more or less not used for edit () (only for the very first initialization). Changing the EDITOR-field in edit has to be done by edit EDITOR "some_editor" not by using EDITOR () which only changes the editor used for edit_history. So maybe we should leave things here as they are and clarify this in the docs.

But I agree that it is confusing that the GUI editor or a custom editor configured in the GUI is called when edit () is evaluated regardless the EDITOR field of edit. I submitted a patch #8240 that synchronizes the editor configuration of edit () and of the GUI. Maybe it helps avoiding this confusion.

Torsten Lilge <ttl>
Group Member
Sat 23 Nov 2013 10:38:32 PM UTC, comment #6: 

The confusing bit is that EDITOR is used for more than just the edit() command.

EDITOR also supports edit_history(), which must not be an "asynchronous" command or else it serves no purpose. In other words, if we want to keep edit_history() (similar to the Unix "fc" command) then EDITOR must block until the file is saved and the editor exits. On the other hand, for the edit() command it is very useful (and now the default) to use an asynchronous editor that comes up in a window and returns control to the Octave shell immediately.

For example, my EDITOR is "vim" and "edit get editor" is "gvim %s".

It's probably true that edit() is used much more than edit_history(), and for GUI users the interactive history panel will be much more useful, but until we deprecate or remove edit_history(), we have this other use of EDITOR to keep in mind.

Mike Miller <mtmiller>
Group Member
Sat 23 Nov 2013 10:35:00 PM UTC, comment #5: 

With the GUI, it seems we have 3 different default editors:

  • the one specified in EDITOR to be used by edit_history


  • the one specified with edit()


  • the one used in the GUI


Isn't that a bit too much?

Carnë Draug <carandraug>
Group Member
Sat 23 Nov 2013 08:32:52 PM UTC, comment #4: 

Isn't this whole issue a bit confusing? I mean, changing EDITOR only works until you try to set it though edit?

Since Matlab's edit does not allow setting the value for EDITOR, I'll suggest to drop that option from our edit. It makes things so confusing... let's have the text editor defined in a single place, and make that place be EDITOR.

Carnë Draug <carandraug>
Group Member
Sat 23 Nov 2013 03:09:21 AM UTC, comment #3: 

Attaching my old patch for this, does this meet the needs? From the user's point of view it will look like changing EDITOR also changes the setting saved inside the edit function, until "edit editor foo" is invoked, after that point the two are configured independently.

(file #29692)

Mike Miller <mtmiller>
Group Member
Sun 17 Nov 2013 08:11:58 PM UTC, comment #2: 

The edit function was one of the first things I looked at when I started contributing to Octave. I got confused quickly with the hierarchical preferences and permutations and gave up on it after a while.

I may still have a patch somewhere that addressed this one, I'll try to find it. I think the consensus was that the default should match EDITOR, but still allow them to be changed independently. I think my patch did something like initialize the persistent value inside edit to an empty matrix. When the function is called, if the value is empty, use the value of EDITOR, else use the persistent value. This would allow edit to track EDITOR if EDITOR is changed, but also allow edit to use a different editor if desired.

They should be allowed to be different, for example I would want EDITOR to be "vim" for editing history in the console, but I would want the edit function to launch "gvim" and return control to the Octave shell.

Mike Miller <mtmiller>
Group Member
Sun 17 Nov 2013 09:53:09 AM UTC, comment #1: 

I can confirm this behavior. The initialization of the field EDITOR in edit is done within a persistent declaration and only is evaluated the first time edit is called.

If edit should take the value from EDITOR() each time edit is called then a value set by "edit EDTIOR some_editor" should also evaluate "EDITOR (some_editor)". Otherwise a customized EDITOR field in edit would be overwritten within the next edit call. If this is desired I could change edit.m in that way.

Torsten Lilge <ttl>
Group Member
Sat 16 Nov 2013 05:37:59 PM UTC, original submission:  

The function edit() states that EDITOR defaults to the value of Octave's EDITOR. However, changing its value after calling EDITOR the first time has no effect.

Example:

octave:1> EDITOR
ans = emacs
octave:2> edit help
octave:3> sh: 1: emacs: not found

octave:3> EDITOR nano
octave:4> edit help
octave:5> sh: 1: emacs: not found


I can imagine that when using the function edit() for the first time, non-emacs users will be caught by surprise. They would then try to change the value of EDITOR and caught by surprise.

On a related note, conflicting documentation exists between EDITOR (which suggests that is only used by edit_history) and edit (which says that uses EDITOR as default).

Carnë Draug <carandraug>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #29696:  fancy-editor.patch added by carandraug (7KiB - text/x-patch)
file #29694:  edit.patch added by carandraug (4KiB - text/x-patch)
file #29692:  edit-editor.patch added by mtmiller (5KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by carandraug (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-12-04 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2013-11-24 carandraug Attached File- Added fancy-editor.patch, #29696
    2013-11-24 carandraug Attached File- Added edit.patch, #29694
    2013-11-23 mtmiller Attached File- Added edit-editor.patch, #29692
        Item GroupNone Incorrect Result
        StatusNone Patch Submitted

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code