bugGNU Octave - Bugs: bug #42307, Play button does not run script if...

 
 

bug #42307: Play button does not run script if name contains hyphen

Submitter:  None
Submitted:  Tue 06 May 2014 02:31:52 PM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
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

Sat 21 Feb 2015 03:12:34 PM UTC, comment #16: 
Torsten Lilge <ttl>
Group Member
Fri 20 Feb 2015 09:44:50 PM UTC, comment #15: 

I just discovered a problem with the patch: When running a file from the editor with breakpoints, entering "dbcont" in the terminal throws the error message


error: dbcont: can only be called in debug mode


if the debug mode is left by this "dbcont". The terminal command "dbstep" as well as continuing via the gui actions are okay (the latter is using Fdbquit).

Torsten Lilge <ttl>
Group Member
Fri 20 Feb 2015 09:21:20 PM UTC, comment #14: 

Matlab doesn't have source, just run().

Rik <rik5>
Group administrator
Fri 20 Feb 2015 03:36:55 PM UTC, comment #13: 

This patch looks good, but I think Fsource should hit breakpoints? I can't think of a situation in which it shouldn't. What does source do in Matlab, does it hit breakpoints or not?

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Thu 19 Feb 2015 09:18:15 PM UTC, comment #12: 

I have pushed the patch with changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/034bcac0b61c

Torsten Lilge <ttl>
Group Member
Wed 18 Feb 2015 08:45:21 PM UTC, comment #11: 

@Torsten: This seems to work for me.  I didn't check the coding sytle or anything else that Jordi might be concerned with though.  I think you can apply it and we can make any further changes directly on the Mercurial sources rather than reviewing and changing the patch on this bug report.

Rik <rik5>
Group administrator
Wed 18 Feb 2015 07:17:27 PM UTC, comment #10: 

Attached a patch for testing that provides a new class octave_cmd with derived classes for normal commands executed in the terminal and for running files. The latter uses Ffeval if the function name is a valid identifier and Fsource if this is not the case.

(file #33120)

Torsten Lilge <ttl>
Group Member
Wed 18 Feb 2015 06:20:34 PM UTC, comment #9: 

"source" does not trigger breakpoints.  I think because it is supposed to be exactly as if you had typed the commands on the keyboard.

Rik <rik5>
Group administrator
Tue 16 Dec 2014 09:02:07 PM UTC, comment #8: 

While working on a new patch using an octave_cmd class I found out that breakpoints are ignored when executing a script via Fsource (). Is it a problem with my build or a general issue?

Torsten Lilge <ttl>
Group Member
Sat 25 Oct 2014 08:14:46 AM UTC, comment #7: 

Jordi, you are right, _cmd_queue does not have to be a pointer.

When thinking about cleaning up the other callbacks and using the queue for further commands (including ones with existing C++ API function) I am not sure how to dispatch all these calls using polymorphism. Regarding an overhaul of the command  mechanism I will have a look at Dan's patch #8016.

Torsten Lilge <ttl>
Group Member
Wed 22 Oct 2014 07:07:57 PM UTC, comment #6: 

Nice, so it really is possible to call Fsource. Now, for a bit of code review...

  • I don't very much like the switch statement on cmd_type. Seems to me like this is what polymorphic dispatch is for. If you do go for polymorphism, then the cmd_info struct would have to become octave_cmd or something like it. This might break API too much and be too much of a pain, and since I'm not the one writing the code, I don't get to make this decision.


  • I never like bare pointers. Does _cmd_queue really have to be a bare pointer? I know this is not really part of this change, just noticed it now.
Jordi Gutiérrez Hermoso <jordigh>
Group Member
Wed 22 Oct 2014 04:53:17 PM UTC, comment #5: 

I have attached a patch for testing that uses Fsource () in the callback function if a file has to be executed.

(file #32311)

Torsten Lilge <ttl>
Group Member
Mon 20 Oct 2014 11:11:06 AM UTC, comment #4: 

Hi,

is it possible to unmark this as fixed? This is not really fixed, per Jordi's comment.

Also, the shell injection possibility is a concern (Comment #2).

Anonymous
Tue 06 May 2014 06:54:29 PM UTC, comment #3: 


> Can't we just avoid attempting to parse the filename at all and
> instead directly call Fsource from as defined from
> libinterp/builtin-defun-decls.h from the build directory? Does the
> octave-link class allow this?


Answering my own question, looks like we could. We could use
octave_link::post_event, which seems to me is able to accept arbitrary
Octave C++ functions and arguments. I'll attempt a patch, but if I
don't have it ready within a day or two after writing this, I will
abandon this and hope someone else does it.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Tue 06 May 2014 06:46:31 PM UTC, comment #2: 

cset 770c525a1a2b doesn't quite address this bug, does it? I mean, even if there is a warning about the script name not being a valid identifier, there's no reason why the GUI should be nagging with pop-ups about this after the fact (I also expect many users will be confused by what "is not a valid identifier" means). The warning is more for the case about trying to run the script from the CLI. Also, this allows for a Octave shell injection in case someone creates a filename with quotes

Can't we just avoid attempting to parse the filename at all and instead directly call Fsource from as defined from libinterp/builtin-defun-decls.h from the build directory? Does the octave-link class allow this? Part of the idea of tying the GUI tightly with Octave is so that we could directly use Octave's general C++ API from the GUI.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Tue 06 May 2014 04:22:56 PM UTC, comment #1: 

Thanks for the report. This issue is already fixed in the gui-release branch (see bug #41136) and will be included in the next major release.

Torsten Lilge <ttl>
Group Member
Tue 06 May 2014 02:31:52 PM UTC, original submission:  

In the editor, create a new document with the contents "disp(1)". Now, save the document as "some-test". Use the play button in the toolbar to run the script.

Result:
error: 'some' undefined near line 1 column 1

Expected result:
1

play button should possibly  run("scriptname") ?

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #33120:  bug42307_octave_cmd_01.patch added by ttl (11KiB - text/x-diff)
file #32311:  bug42307.diff added by ttl (6KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by None (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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-02-20 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2015-02-19 ttl StatusPatch Submitted Ready For Test
    2015-02-18 ttl Attached File- Added bug42307_octave_cmd_01.patch, #33120
    2014-10-22 ttl Attached File- Added bug42307.diff, #32311
        StatusFixed Patch Submitted
        Open/ClosedClosed Open
        Release3.8.1 dev
        Operating SystemGNU/Linux Any
    2014-05-06 ttl StatusNone Fixed
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code