bugGNU Octave - Bugs: bug #39179, Warn or fall back to external...

 
 

bug #39179: Warn or fall back to external editor if built without QScintilla

Submitted by:  Dan Sebald <sebald>
Submitted on:  Thu 06 Jun 2013 04:16:37 PM UTC  
 
Category:  GUI Severity:  1 - Wish
Priority:  3 - Low Item Group:  Feature Request
Status:  Fixed Assigned to:  Torsten Lilge <ttl>
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Mon 05 Jun 2017 11:33:28 AM UTC, comment #23: 

no more issues reported -> closing as fixed

Torsten Lilge <ttl>
Project MemberIn charge of this item.
Tue 11 Apr 2017 08:29:06 PM UTC, comment #22: 

I have pushed a new version of the patch with cset
http://hg.savannah.gnu.org/hgweb/octave/rev/41639665aa34

Torsten Lilge <ttl>
Project MemberIn charge of this item.
Tue 11 Apr 2017 06:20:16 PM UTC, comment #21: 

That seems to be working fine on my machine.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 11 Apr 2017 05:30:19 PM UTC, comment #20: 

And here the attachment ...

(file #40361)

Torsten Lilge <ttl>
Project MemberIn charge of this item.
Tue 11 Apr 2017 05:29:30 PM UTC, comment #19: 

Dmitri, I have attached a new patch that does not cause the freeze on my system. Would you mind to test it with your built configuration before I push it?

Torsten Lilge <ttl>
Project MemberIn charge of this item.
Sun 09 Apr 2017 07:57:19 PM UTC, comment #18: 

I can confirm the effect and am trying to find the responsible changeset.

Torsten Lilge <ttl>
Project MemberIn charge of this item.
Sun 09 Apr 2017 07:09:57 PM UTC, comment #17: 

This was for non-gui version
./run-octave --no-gui
octave:1> quit
(octave hangs)

A gui version seems to be working.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 09 Apr 2017 07:05:46 PM UTC, comment #16: 

I think those recent changes broke octave.
Now after "quit", octave just hangs with 100% CPU.

Dmitri.

Dmitri A. Sergatskov <dasergatskov>
Sun 09 Apr 2017 06:27:59 PM UTC, comment #15: 

Changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/2cee3976d88f
move the open dialog for editing files into the main window and is therefore also available when octave was built without qscintilla.

Torsten Lilge <ttl>
Project MemberIn charge of this item.
Sun 09 Apr 2017 06:52:14 AM UTC, comment #14: 

I have pushed changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/7332287221a9
which moves some functionality for opening files from the editor into the main window for being available even when octave is built without qscintilla. Moreover, interfacing the external editor is handled by a new class.

Torsten Lilge <ttl>
Project MemberIn charge of this item.
Sun 01 Feb 2015 04:35:40 PM UTC, comment #13: 

I'm updating the description and goal of this dormant bug.

The current default branch still silently does nothing if the user tries to "edit filename" from the command window in the GUI.

The GUI already has a facility for launching an external editor if the user's preferences say so. The file_editor class checks the preference and opens the file itself or launches the external program when it gets the edit_file_signal. We do want to support this, so we can't just return false from do_edit_file if we have no QScintilla.

If the GUI is built without QScintilla, there could either be some kind of warning that the built-in editor can't be used, or it could automatically fall back to forcing the external editor preference to be used by default.

Mike Miller <mtmiller>
Project Administrator
Thu 23 Jan 2014 07:57:17 AM UTC, comment #12: 

I understand what you mean, and it is tempting to do that because of how simple it is, but the approach just doesn't feel right.  Maybe someone should try to make the editor more generic and encapsulate any non-qscintilla editor in a Qt window/tab similar to what QScintilla behaves right now, at the same time make the editor selectable via the "edit" script.  Any encapsulated editor should have a means to accept debugging signals (even if the editor doesn't do anything with the signals).  In that case, QScintilla is just another of many possible editors.

I suppose this bug could be closed.  A good solution isn't straightforward.

Dan Sebald <sebald>
Mon 20 Jan 2014 02:08:54 PM UTC, comment #11: 

I guess I wasn't making myself clear. Dan, if you look at the current state of the GUI it already has a setting to configure launching an external editor, this has already been done. Now if the built-in editor is not made because of a missing qscintilla, users may be confused that the preference that says "launch external editor" is not respected. Launching the configured external editor seems like the easiest and most straightforward approach given what the GUI does today.

I have no opinion which way is right way, I'm just throwing out an idea that seems clear and respects the GUI user's preferences.

Mike Miller <mtmiller>
Project Administrator
Mon 20 Jan 2014 07:06:46 AM UTC, comment #10: 

Oh, I'm going to say, No.  At least not what I want to contribute to supporting right now.  I suppose it is possible to have the GUI launch a different editor, but honestly there is something self-defeating about that unless the editor in question is properly integrated into the environment.  Otherwise, it makes me want to say, All that way for this?  Editor XYZ in it's own window already existed before the GUI.

Wouldn't it make more sense to put effort into improving the QScintilla editor?  The one area where the GUI clearly makes life a bit easier for the user compared to CLI is debugging, at least if it is done well.  So, I'd put priority on an integrated editor that handles breakpoints seamlessly, tooltips with variable values, etc.  Can that be done with vim, emacs, etc.?

Dan Sebald <sebald>
Mon 20 Jan 2014 06:33:17 AM UTC, comment #9: 

No I was thinking in the GUI itself, no need to communicate something back to the interpreter. The GUI now has a preference to launch an external editor, separately from the way the editor is launched in CLI mode. If the GUI has no scintilla it could simply launch the external editor.

Mike Miller <mtmiller>
Project Administrator
Mon 20 Jan 2014 06:10:42 AM UTC, comment #8: 

Yes, that was the idea of the patch/changeset.  I.e., that the GUI sends back a response that an editor tab was not opened.  The script can then appropriately respond by opening its own editor, whatever preference that is.

The issue is that we need a solid paradigm for communicating data across the threads.  It's hard to add new features in an elegant way without that.  To this point we've avoided using mutex's although there seems no good solution without them.  They're essential for communicating data from worker (Octave core) to GUI.  (Worker must suspend momentarily.)  Going the other direction, there might be an alternative which is to have functions of Octave core overrided similar to object-oriented programming.  In such a scheme the GUI would launch the worker and then install it's own functions like "edit" and so on.  Otherwise, the normal edit would happen.  The latter concept I'm interested in from a see-what-happens sort of perspective, but the mutex data exchange I'm pretty much sold on.

Dan Sebald <sebald>
Mon 20 Jan 2014 04:12:16 AM UTC, comment #7: 

Just jumping in here, but would it make sense to have the GUI launch the external editor preference and make that be the default if the GUI was built without QScintilla?

Mike Miller <mtmiller>
Project Administrator
Sun 09 Jun 2013 09:18:11 PM UTC, comment #6: 

One more try:

>> edit plot
error: GUI not responding
error: called from:
error:
/usr/local/src/octave/octave-editfile/build1/../octave/scripts/miscellaneous/edit.m
at line 530, column 10
error:
/usr/local/src/octave/octave-editfile/build1/../octave/scripts/miscellaneous/edit.m
at line 335, column 9

Dan Sebald <sebald>
Sun 09 Jun 2013 09:12:57 PM UTC, comment #5: 

I must have mistyped.  Here's the sample error message:

Dan Sebald <sebald>
Sun 09 Jun 2013 09:10:42 PM UTC, comment #4: 

Oh yeah, about the 10 second timeout.  The GUI basically has ten seconds to find the file and open it.  Removing the time input option would make the wait indefinite.  That too would probably be fine, because I haven't seen too many problems with this approach.  I just wanted to test the concept of the link timing out.  If ever the GUI gets stuck for some strange reason while opening a file, the following example error appears:

Dan Sebald <sebald>
Sun 09 Jun 2013 09:06:31 PM UTC, comment #3: 

Thank you!  (That behavior was annoying.)

I've attached a changeset that will return various results for the GUI open file function and pass that back to the edit.m script.  This is done using a mutex and illustrates a general approach to freezing the worker process momentarily while the GUI does some work.  Please try it out and see what you think, and give some thought about this general process because I've used this strategy in other places.  One thing I've added is to lock the mutex before going into a wait state, then on the other side try to lock the mutex as a means to verify the worker process has actually gone into a sleep state.  (The way a mutex works is that even if it is in a lock state, it gives up its lock and becomes unlocked when in the sleep state.)  I'm wondering if this may be overdoing the "hand-shaking" a bit because on other examples I've run millions of trials with and without the extra lock attempt and neither ever faltered.

Note that this patch solves the bug of do_edit_file () always returning true, but beyond that the behavior under certain conditions might not be worked out as desired.  For example, try:

edit plot

then

edit foodoo

In the case where there is no file, the GUI currently indicates no success, then edit.m responds by opening an empty file of that name using the custom editor.  If that behavior is to be altered in some way (GUI opens empty file, edit.m gives warning about file not found), then that is a different changeset.

As to your question about whether do_edit_file () should actually be run, I think probably so although I'm not of strong opinion on that.  There are two distinct philosophies to this, as I see it:

1) Use "#ifdef HAVE_QSCINTILLA" liberally to weed out all code that isn't necessary for proper operation.  This is the approach of the changeset because that seemed the way things were tending.  There is the advantage of reducing unused code hunks when the feature isn't needed.  However, there are a couple drawbacks to this.  One is that there gets to be a lot of "#ifdef HAVE_QSCINTILLA" placed about in a half dozen files or so.  Sometimes it is nice to keep that limited to just one or two files (the second philosophy).  A second is that the action this code performed in the presence of QScintilla is no longer available to do other things.

2) The second philosophy is to carry out all the same code to the very last possible point where a decision can be made as to whether to call the QScintilla code.  In that scenario in only one or two files will there be "#ifdef HAVE_QSCINTILLA", probably somewhere in file-editor.cc near:

          file_editor_tab *fileEditorTab = new file_editor_tab ();

and just pass back a code that means GUI editor support is not present.

The reason I prefer philosophy 2 in this case is that I like not having so many instances of HAVE_QSCINTILLA scattered about the code, but more than that (if I understand correctly) there is currently a way for the GUI to use a custom editor with "open" menu.  I think, but I may be wrong, with the current setup where all this supporting code is extracted when there is no HAVE_QSCINTILLA then even that custom editor can no longer be used.  The custom editor is the first option inside of file_editor::request_open_file.  That is why I think isolating things to one instance of HAVE_QSCINTILLA is good.  (If folks agree, let's do that as a separate followup patch so that we have a record of both approaches.  Shouldn't be a difficult change.)

(file #28285)

Dan Sebald <sebald>
Sun 09 Jun 2013 03:39:06 PM UTC, comment #2: 

I found out that

edit xxx.m

does not automatically show the editor (if hidden) or set the focus to the editor. Changeset http://hg.savannah.gnu.org/hgweb/octave/rev/6960ac6ed127 fixes this.

Concerning the function do_edit_file (), should it really being called if QScintilla was not built for the system or should this case be handled as if no gui is available?

Torsten Lilge <ttl>
Project MemberIn charge of this item.
Fri 07 Jun 2013 03:42:24 AM UTC, comment #1: 

Hm that "someone" was me.
Note that I do have QScintilla & libqscintilla installed, built for my system.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 06 Jun 2013 04:16:37 PM UTC, original submission:  

Someone submitted a problem to the discussion list about nothing happening when "edit xxx.m" is typed at the command line.  The issue is that even though QScintilla was not built for the user's system, octave_qt_link::do_edit_file () is always returning true so that "edit" script doesn't realize the file was not opened so that it can act accordingly.

The minimal change should be something like the following:

bool
octave_qt_link::do_edit_file (const std::string& file)
{
#ifdef HAVE_QSCINTILLA
  emit edit_file_signal (QString::fromStdString (file));
  return true;
#else
  return false;
#endif
}

but in the next week or so I will submit a changeset that will add an exchange of signals whereby this routine emits the signal and then waits to be awaken with true/false about the success of opening the file.  We'll see how that behaves and if there is any chance of the system freezing indefinitely we'll fall back on the above, but it shouldn't be too difficult.

Dan Sebald <sebald>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #40361:  external_editor_interface.patch added by ttl (38KiB - text/x-patch)
file #28285:  octave-edit_file-2013jun09.patch added by sebald (8KiB - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

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

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-06-05 ttl StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2017-04-11 ttl Attached File- => Added external_editor_interface.patch, #40361
    2017-04-09 ttl StatusIn Progress => Ready For Test
    2017-04-08 ttl StatusNeed Info => In Progress
        Assigned toNone => ttl
    2016-11-23 mtmiller Dependencies- => bugs #49680 is dependent
    2015-02-01 mtmiller Severity3 - Normal => 1 - Wish
        Priority5 - Normal => 3 - Low
        Item GroupIncorrect Result => Feature Request
        Summaryoctave_qt_link::do_edit_file always returns true even when QScintilla not present => Warn or fall back to external editor if built without QScintilla
    2014-01-20 mtmiller StatusNone => Need Info
    2013-06-09 sebald Attached File- => Added octave-edit_file-2013jun09.patch, #28285

    Back to the top


    Powered by Savane 3.5