bugGNU Octave - Bugs: bug #47585, GUI editor does not show already...

 
 

bug #47585: GUI editor does not show already active breakpoints when a file is opened

Submitter:  Mike Miller <mtmiller>
Submitted:  Thu 31 Mar 2016 03:38:05 PM UTC
   
 
Category:  GUI Severity:  2 - Minor
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

Mon 15 Oct 2018 06:02:24 PM UTC, comment #22: 

Dan, thanks for testing. Closing the report.

Remaining callbacks in the gui code will be replaced by using the command queue (with or without return values) step by step.

Torsten Lilge <ttl>
Group Member
Sat 13 Oct 2018 04:01:39 PM UTC, comment #21: 

The changeset appears to work and is a good design.  This report can be closed...  Consider using the octave_value_list via signal/slot for the many other similar methods in the GUI; on a development branch perhaps.

I'm not sure how the current approach works, but there would need to be some type of "update" signal/slot.  That is, every time the core worker process does something there is a potential that the information in the GUI may have changed, say in the variable editor.  It would be nice if there were some way for objects to dynamically connect to this "update" signal so that they could then inquire via this octave_value_list signal/slot for any updates.  (I would investigate some means of asking only for that variable information which is visible in the variable viewer, not say the whole big underlying matrix.)

Consider, though, the distinction between a user-driven change and an internal state changed.  For example, "dbstop" is a user-driven state change.  In the short term, it would be alright to keep inquiring about breakpoints after each processing interval (or use whatever the current mechanism is), but in the long run I think it would work much more efficiently if there were some way of "installing" custom function overrides, say


GUI_dbstop
{
    call builtin dbstop;
    inquire about updated breakpoints via dbstatus;
}


With such a construct the core worker process would be completely oblivious to the need to update breakpoint info in the GUI.  That GUI_dbstop would be a custom routine in the interpretter's function table somewhere.

Dan Sebald <sebald>
Tue 09 Oct 2018 05:34:23 PM UTC, comment #20: 

A good point, just using arbitrary file paths for comparisons in fact no good idea. The reason for not needing QFileInfo here might be that the path strings are already canonicalized (e.g. before loading a file etc.)

Torsten Lilge <ttl>
Group Member
Tue 09 Oct 2018 04:29:22 PM UTC, comment #19: 

I couldn't seem to devise any link situation that caused a problem, so the QFileInfo(str1) == QFileInfo(str2) is probably not needed.  Whenever a link it opened, the file name gotten by the editor is the original file, i.e. "edit tf.m" will open the file named "testfun.m".

Dan Sebald <sebald>
Tue 09 Oct 2018 06:11:01 AM UTC, comment #18: 

Here's a means to check if two file names refer to the same file, if it can be useful:

http://doc.qt.io/qt-5/qfileinfo.html#operator-eq-eq

I.e., simply create some QFileInfo objects using those strings as arguments, then do the == comparison.

Dan Sebald <sebald>
Tue 09 Oct 2018 06:02:35 AM UTC, comment #17: 

@Torsten

comment #13: Ah, so that "destination object" information is inherently package as part of the command object whereby a connection is made during construction.  And once that command is processed, it is deleted which Qt automatically destroys the signal/slot connection?  Nice.

comment #15: OK, so you're thinking just let the interpreter/parser resolve all the path name information and provide the final name.  If it matches, then that's it.  I can see the logic in that, but doesn't one have to be careful with links and so on?  That is:


        if (file (i).string_value () == _file_name.toStdString ())


is comparing string names, but it isn't comparing the files in an OS sense.  That is, could the strings be referring to the same file even though they are different, if one of them is a link?

Dan Sebald <sebald>
Tue 09 Oct 2018 05:08:16 AM UTC, comment #16: 

@Dan: Thanks for the comments.

comment #13: The uniqueness, regardless analyzing the filename in the dbstatus case (commenct #14), is given by passing object pointer and slot to the worker thread. By this, the signal is only connected to this unique caller.

comment #15: By omitting the function name I intended to avoid ambiguities in the filename/function relation by subfunctions etc.

Torsten Lilge <ttl>
Group Member
Mon 08 Oct 2018 10:24:12 PM UTC, comment #15: 

Did you program this to inquire db info just about the file in question?  That is, one could specify just the name of the file (to be) opened:


>> dbout = dbstatus("testfun")
dbout =

  2x1 struct array containing the fields:

    name
    file
    line
    cond


And presumably make things more efficient if there are already a couple dozen files open with all sorts of breakpoints.  Maybe when opening the GUI Editor for the first time one would use "dbstatus()" to get info about every file so that the command-slot is called just once in that case.

Dan Sebald <sebald>
Mon 08 Oct 2018 10:05:12 PM UTC, comment #14: 

Oh, you are using the file information from the command itself as a way to direct the breakpoint information and avoid confusion:


  void file_editor_tab::update_breakpoints_handler (const octave_value_list& argout)
  {
    octave_map dbg = argout(0).map_value ();
    octave_idx_type n_dbg = dbg.numel ();

    Cell file = dbg.contents ("file");
    Cell line = dbg.contents ("line");
    Cell cond = dbg.contents ("cond");

    for (octave_idx_type i = 0; i < n_dbg; i++)
      {
        if (file (i).string_value () == _file_name.toStdString ())
          do_breakpoint_marker (true, this, line (i).int_value (),
                                QString::fromStdString (cond (i).string_value ()));
      }
  }


Such a good construct, decomposing that "octave_map" and "contents".  This fits together nicely.

Dan Sebald <sebald>
Mon 08 Oct 2018 09:38:39 PM UTC, comment #13: 

The changeset appears to work on a little testfun() I wrote, placed breakpoints (command line and point/click), closed and opened.  The approach is the way to go, as I see it.  A couple things to keep in mind that may be of importance but aren't so significant because they are easily rectified at a later time.

1) Qt-related: Make sure there is a uniqueness to the information being sent back and forth asynchronously (via the signal/slots mechanism) so that the GUI doesn't get confused by multiple objects requesting information.  E.g., say multiple files are all opened at one time and they all request breakpoint info.  Is it possible that information requested about one file could end up going to some other file?  The way I had addressed this was quite simple: tag the command using the object's "this" pointer as an ID then when the information comes back through the slot check that the ID-tag matches "this".

2) More general: Is the "emit argout_signal (argout);" placed in the code at a location where it is Qt related and not more general interface or core code?  That is, this concept of calling a command for information would hold for other third-party frameworks so long as the interface allowed it.  So it would be nice if this weren't restricted to, say, this Qt implementation only.  I think that aspect of it can be cleaned up later without too much change to where you placed the "emit"--need some thread considerations and such, but still not too bad.

Dan Sebald <sebald>
Mon 08 Oct 2018 08:26:12 PM UTC, comment #12: 

I have pushed changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/a7511a1489b8
which should fix this bug.

It implements an interface for the command queue that allows the GUI thread to call builtin functions in the worker thread and get the return values. The method uses a cross thread signal/slot mechanism as Dan's approach from some time ago but does not change the general mechanism for calling commands in the worker thread via the command queue.

Torsten Lilge <ttl>
Group Member
Thu 16 Aug 2018 05:06:44 AM UTC, comment #11: 

This bug is still present.  I used the following code


dbstop ls
edit ls
## Verify, breakpoint is shown in file on first opening
## Close file in editor
dbstatus
edit ls
## Breakpoint is no longer shown, but still exists according to dbstatus



Rik <rik5>
Group administrator
Tue 12 Apr 2016 07:06:27 PM UTC, comment #10: 

See some follow-up as regards comment #2 in bug #33411

Philip Nienhuis <philipnienhuis>
Group Member
Mon 11 Apr 2016 04:24:45 AM UTC, comment #9: 

After looking at Dan's framework again, I've decided I won't have a go at this after all.  I don't think I understand well enough what should be signals and what should be function calls.

Dan, when you have written up how things should work, I may revisit this.

Lachlan Andrew <lachlan>
Mon 11 Apr 2016 03:20:57 AM UTC, comment #8: 

Dan, I agree entirely with your first sentence in comment #7.

If you have clear idea of what the "proper framework conventions" should be, please document them.  I'll be happy to try to work within it, even if it is by writing kludge shims to be replaced when the proper framework is developed as you suggest.  I just have don't really know what the framework should look like.  For this particular bug, I'll have a go at what you suggested, but it would be nice to have something more general written down.

Arguing about whether or not a cross-thread fix makes the code "less buggy" is a bit like arguing whether or not Lewis Carol was right that a clock with stationary hands is "better" than one that looses one minute per day.  (The one with stationary hands is exactly right twice a day, which is much more often than the other one is right, but the one with moving hands is more useful.)



The structure returned by all functions is an octave_value_list.  I'm not convinced that the best approach is simply to allow CLI commands to be run by the GUI.  It would be useful to have a more generic thread-safe way to make a core function callable from the GUI. You have pointed out that this is harder to debug than the more restrictive CLI-like interface.  There's always a tradeoff, with multiple useful combinations.

Lachlan Andrew <lachlan>
Mon 11 Apr 2016 12:38:26 AM UTC, comment #7: 

Indenting and identifying parse errors would save a lot of user hours, it's just that it would be easier to do these things if there were a nice framework for doing so.

It's a caught-between-a-rock-and-a-hard-place type of situation, really.  The major contributors have to be the ones to implement/test some generic way of GUI/core, cross-thread communication.

As I see it, the program isn't made less buggy by calling core routines directly across a thread.  At worst, it is leaving a potentially difficult bug for some future user to understand, plus not a path to robust, dependable code.  It would also be nice to stay withing the GUI's framework.  If anything, isolating dodgy code to some hack or kludge file would be good.  Say, creating some bogus command process that calls "dbstatus('justopenedfile')" command for which the framework could send a signal and get back an octave_value_list.  That is,



octave:14> edit print
octave:15> dbstop plot 216
ans =  216
octave:16> dbstop plot 229
ans =  229
octave:17> dbstop print 357
ans =  357
octave:18> brklist = dbstatus('plot')
brklist =

  scalar structure containing the fields:

    name = plot
    file = /usr/local/share/octave/4.1.0+/m/plot/draw/plot.m
    line =

       216   229


That way, the file-editor-tab.c/h files could be programmed to the proper framework conventions and the dodgy cross-thread code could be kept somewhere else for eventual replacement.  From what I remember, that returned structure for dbstatus is in an octave_value_list.

Dan Sebald <sebald>
Sun 10 Apr 2016 10:51:28 PM UTC, comment #6: 

Dan, I wasn't meaning to be dismissive by calling the design principles "niceties".  (I meant it in the sense of "requiring careful treatment" rather than "fine points".)  I agree that they are important in the long term.

However, I don't see the need to have Octave buggy while we are waiting for the proper framework to be developed.  Perhaps it would be good to have a comment at each place the cross-thread actions happen so that it is easy to fix them once the correct mechanisms are available.  There were already instances of that within the debug/dbstop code before I started working on it.

I share your concerns on some matters.  I would like the editor to have access to the parse tree, to get things like folding and auto-indent working properly, and to get a "warning bar" like Matlab has in the RHS gutter.  I am resisting because I don't consider them sufficiently critical to fudge the cross-thread interaction you are working to allow.

If there is something I can do to get the right architecture in place, let me know.  However, I wouldn't know where to start.

Lachlan Andrew <lachlan>
Sun 10 Apr 2016 09:26:52 PM UTC, comment #5: 

I wouldn't call what I'm proposing "niceties"; more like a design principle.

The designers of Qt came up with the signals and slots concept because they often found the callback routine approach to be limiting and sometimes became a bottleneck.  There is discussion about this on the web if one does a search for Qt and callbacks.

I can understand the desire for making the breakpoints work, but I wouldn't advocate this patch.  One has to keep in mind that the GUI is running in a different thread and calling core routines asynchronous with what the core may be doing requires some type of semaphore or mutex.  Otherwise, there is a the remote possibility of the core changing something, in this case, in the breakpoint table, at the same time the GUI is trying to access it.  And these types of obscure bugs are really difficult to locate because they are near impossible to reproduce when someone reports a problem.

I see a lot of quick-fix, workarounds that push a problem into some other corner but never completely solve the problem in graphics, breakpoints, dialog boxes, etc.  If all that effort had been put into a good design principle early on, it would have been more productive.

Dan Sebald <sebald>
Fri 08 Apr 2016 05:29:49 AM UTC, comment #4: 

For a while now, I have been using the attached patch.

It ignores the architectural niceties that Dan points out, by calling "core" functions directly from the GUI.

I've noticed some breakpoint-related oddities recently (mainly with breakpoint conditions changing unexpectedly) which may be related to this patch, but I haven't had a chance to track them down.

(Philip, I have posted a fix for bug #33411.  If you run into that problem often, let me know if the patch helps.)

(file #36860)

Lachlan Andrew <lachlan>
Mon 04 Apr 2016 02:19:14 AM UTC, comment #3: 

Yes, related, but only in the sense that there needs to be communication in both directions across the GUI "boundary", be it a thread or some other configuration.  I put together a detailed description at one point, classifying the various actions in the whole framework.

When it is the GUI side initiating action, there is a wealth of commands for getting breakpoints, getting variables, setting variables, getting properties, setting properties, etc.  All these can be accessed via some command/return I described in a previous post.

Then there is the case where an action is initiated in the core--or maybe I should say initiated from the command line via the core.  What's a nice way of handling that?  A way that doesn't require writing all kinds of GUI-specific code in the core?  A way that any number of GUI frameworks could make use of?  I propose a means to "override" the command-line function very similar to the way C++ derived classes can override the base class function (and still call that function if necessary).  I would imagine that almost exists already because we can create our own functions having the same name as some existing function in Octave.  The thing that is missing is the ability to still call the built-in functions somehow.

The GUI when launched would install its own set of functions that need to cause some kind of GUI action from the command line.  As an example:

INSTALLABLE_FUNCTION_CLASS(dbclear) qt_gui_dbclear(void)
{
  BASE_FUNCTION(dbclear);
  send_signal_to_GUI_to_update_breakpoints();
}

Something like that.  GUI frameworks other than Qt could make use of this very generic layout in the same way.

Dan Sebald <sebald>
Sun 03 Apr 2016 08:54:37 PM UTC, comment #2: 

Maybe related, maybe also a little related to bug #33411, but I see the other way round as well (when testing again for bug #33411) with the dev version:

function [ ret ] = foo ()
   printf ("Hello!\n");
endfunction


>> which foo
'foo' is a function from the file C:\Users\philip\MyDocs\octave\foo.m
>> edit foo.m      ## Appears in GUI editor
>> dbstop foo 2
ans =  2           ## breakpoint appears in GUI editor
>> foo
stopped in C:\Users\philip\MyDocs\octave\foo.m at line 2
2:    printf ("Hello!\n");
debug> dbcont
Hello!
>> run foo         ## This is bug 33411. Does not stop
>> dbclear foo
>> foo
Hello!
>>


but after the "dbclear foo" command the breakpoint is cleared (foo.m runs to the end) but in the editor the breakpoint is still visible. It cannot be cleared either (neither with "toggle breakpoints", "clear all breakpoints" or clicking on it).
Only exiting the editor (or closing the foo.m editor tab) and reopening foo.m in the editor helps.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 03 Apr 2016 05:53:06 AM UTC, comment #1: 

I pointed out several years ago that this would be a problem because the callback functions for breakpoints and such is driven from the core side of things and the core does not know the editor has opened a file.  The only thing really needed for all sorts of functionality is a way to send commands to the core and eventually get an octave_value_list return value for that processed command.  In this case, the editor would initiate a "dbstop" and when the signal (not a function call, but a signal) comes back from that processed command with the list of breakpoints in an octave_value_list then the editor would add the breakpoints.  See

https://savannah.gnu.org/patch/?func=detailitem&item_id=8016#options

Dan Sebald <sebald>
Thu 31 Mar 2016 03:38:05 PM UTC, original submission:  

When a file with breakpoints already set is closed and then re-opened in the editor, active breakpoints are not shown but are still active.

Example:


>> edit foo.m
## add some code and save
## add a breakpoint, visually or command line
## verify the breakpoint is shown, dbstatus lists breakpoint
## close foo.m in the editor
## reopen foo.m from the recent files list
## verify no breakpoint is shown
## switch back to command window
>> dbstatus foo
## output shows breakpoint is still active
>> foo
## breakpoint is hit
## switch back to editor, step icon is visible, breakpoint icon is still not


Mike Miller <mtmiller>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-15 ttl StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-10-08 mtmiller Carbon-CopyRemoved 80942 -
    2018-10-08 ttl StatusConfirmed Ready For Test
    2018-08-16 rik5 StatusNone Confirmed
    2016-04-11 lachlan StatusPatch Submitted None
    2016-04-10 lachlan StatusNone Patch Submitted
    2016-04-08 lachlan Attached File- Added bug_47585_load_breakpoints.cset, #36860

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code