bugGNU Octave - Bugs: bug #46795, dbstop lacks much Matlab...

 
 

bug #46795: dbstop lacks much Matlab functionality

Submitter:  Lachlan Andrew <lachlan>
Submitted:  Sun 03 Jan 2016 06:58:50 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Lachlan 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 20 Feb 2016 01:53:58 PM UTC, comment #96: 

I have pushed the csets and am going to file a bug for the update-directory-issue.

Torsten Lilge <ttl>
Group Member
Fri 19 Feb 2016 09:21:13 PM UTC, comment #95: 

Ah good one.  No, I haven't pushed :-(

Torsten, would you please push both fixes? I'm too hazy tonight.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 19 Feb 2016 09:11:23 PM UTC, comment #94: 

Thanks for all your work, Philip and Torsten.

Have these patches been committed?  If not, could one of you please do that?

(Philip, you asked recently if I have been given commit privileges.  I have been given permission to edit Savannah, but asked not to commit any patches -- I tend to be a bit rough-and-ready and they rightly want to have all my changes reviewed :)

Lachlan Andrew <lachlan>
Fri 19 Feb 2016 06:41:41 PM UTC, comment #93: 

Yep this patch prevents the LMB-in-left-editor-margin issue. Thank you very much, Torsten.

I have set the status to "fixed" as dbstop etc. work fine now.

Is there / should there be a bug report for the remaining issue?

Philip Nienhuis <philipnienhuis>
Group Member
Thu 18 Feb 2016 09:40:34 PM UTC, comment #92: 

The attached patches fix the crash on my Windows 7. The order for applying the patches is

  1. backout_e8c3590da9ff_v3.patch  (backing out my patch)
  2. bug_46795_v5.patch   (backing out the use of QMessagebox)


If you can confirm that these patches are working, can we then close the report? The remaining issue (changing directory when setting a breakpoint or running a file, which is not in the path, is not updated in the gui) is not related to the extension of the dbstop features.




(file #36391, file #36392)

Torsten Lilge <ttl>
Group Member
Sun 14 Feb 2016 08:51:47 PM UTC, comment #91: 

Sorry again from my side. I forgot to do the last "hg qref" before exporting the patch. Before making you still more work I am going to test the right patch on windows myself.

Torsten Lilge <ttl>
Group Member
Sun 14 Feb 2016 07:23:56 PM UTC, comment #90: 

I get more or less the same error as with the original cset:


:
  CXX      libgui/src/libgui_src_libgui_src_la-octave-cmd.lo
../dev_dbstop/libgui/src/m-editor/file-editor-tab.cc: In member function 'void file_editor_tab::handle_context_menu_break_condition(int)':
../dev_dbstop/libgui/src/m-editor/file-editor-tab.cc:336:11: error: '_main_win' was not declared in this scope
      || !(_main_win->get_octave_qt_link ()->file_in_path (info.file, info.dir)))
           ^
Makefile:16446: recipe for target 'libgui/src/m-editor/libgui_src_libgui_src_la-file-editor-tab.lo' failed
make[2]: *** [libgui/src/m-editor/libgui_src_libgui_src_la-file-editor-tab.lo] Error 1


IMO it is preferable to sort out the issue more thoroughly than to just apply temporary patches.
After all this is a bug in the Windows development version; I think there are just a handful people using that,

Philip Nienhuis <philipnienhuis>
Group Member
Sun 14 Feb 2016 06:18:29 PM UTC, comment #89: 

OK Torsten, I'll try.

I think it is better to make a debug mxe build (w/o stripping) as well, I'll try that too.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 14 Feb 2016 03:14:50 PM UTC, comment #88: 

Attached is Lachlan's patch with a small correction such that the build finishes successfully.

(file #36336)

Torsten Lilge <ttl>
Group Member
Sun 14 Feb 2016 01:50:20 PM UTC, comment #87: 

Torsten:
no problems :-)
I expect it'll be a tiny issue, as usual in these cases.
Debugging on Windows is a problem for all of us as long as we can't build Octave natively.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 14 Feb 2016 12:14:52 PM UTC, comment #86: 

No worries, Torsten. Collaborative debugging is sort of fun :)

For reference I'm attaching my  file-editor-tab.cc with the debugging info.  I'm not sure if that will work in your system.

The backout patch was with a few other patches installed, such as the one to fix the bug that Torsten pointed out.  It's getting a bit late here; tomorrow I'll try to produce a clean backout from the current tip.

(file #36332)

Lachlan Andrew <lachlan>
Sun 14 Feb 2016 11:39:05 AM UTC, comment #85: 

I am really sorry for having caused this. Moreover I do not have any time this weekend to take care of this.

Torsten Lilge <ttl>
Group Member
Sun 14 Feb 2016 11:33:28 AM UTC, comment #84: 

No joy with just the backout_e8c3590da9ff.cset patch:

:
  CXX      libgui/src/m-editor/libgui_src_libgui_src_la-marker.lo
../dev_dbstop/libgui/src/m-editor/file-editor-tab.cc: In member function 'void file_editor_tab::handle_context_menu_break_condition(int)':
../dev_dbstop/libgui/src/m-editor/file-editor-tab.cc:336:11: error: '_main_win' was not declared in this scope
      || !(_main_win->get_octave_qt_link ()->file_in_path (info.file, info.dir)))
           ^
  CXX      libgui/src/libgui_src_libgui_src_la-main-window.lo
  CXX      libgui/src/libgui_src_libgui_src_la-octave-cmd.lo
Makefile:16446: recipe for target 'libgui/src/m-editor/libgui_src_libgui_src_la-file-editor-tab.lo' failed
make[2]: *** [libgui/src/m-editor/libgui_src_libgui_src_la-file-editor-tab.lo] Error 1


Torsten's patch that gave rise to the LMB-thingy was quite big and invasive one, so I'm not surprised that it isn't easy to "neutralize" it.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 14 Feb 2016 11:00:16 AM UTC, comment #83: 

Thanks for the new patches.
(This is going to look like our pet bug isn't it (with textscan at #2))

The 2nd, debug, patch doesn't apply.

applying ../dbstop/bug_46795_debugging.cset
patching file libgui/src/m-editor/file-editor-tab.cc
Hunk #1 FAILED at 935
Hunk #2 FAILED at 945
Hunk #3 FAILED at 952
3 out of 3 hunks FAILED -- saving rejects to file libgui/src/m-editor/file-editor-tab.cc.rej


I first applied backout_e8c3590da9ff.cset (first 2 hunks had -7 lines offset), then bug_46795_debugging.cset.

Looking at the .rej file it seems your repo must be at quite another revision level than me.

My local repo is at:
[philip@deskprn default]$ hg -v summary
parent: 21254:076876f0a83a
 build: Declare dependency on octave-config.h for generating *.df files
branch: default

I'll start a build with just the first backout patch to see if the LMB-in-editor bug is gone.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 14 Feb 2016 02:24:47 AM UTC, comment #82: 

Philip, at the risk of adding clutter, I'm attaching a patch that backs out Torsten's changes without disturbing more recent changes.

I've also attached a patch that gives some debugging output (bug_46795_debugging.cset).  Could you apply that and tell us what the output is?

Torsten, I have two questions about the patch:
1. why is it necessary to call this after Fcd?  Wouldn't it be better to make Fcd trigger update_directory too?
2. Just to be defensive, should we replace constructs like


  if (_main_win->get_octave_qt_link ()->file_in_path


by


  if (_main_win && min_win->get_octave_qt_link ()
      && _main_win->get_octave_qt_link ()->file_in_path


as a matter of routine?

(file #36328, file #36329)

Lachlan Andrew <lachlan>
Sun 14 Feb 2016 01:43:23 AM UTC, comment #81: 

Great detective work, Philip.

I'll have a look to see if I can think of what may be going on, but will probably have to leave the real work to Torsten.

Lachlan Andrew <lachlan>
Sat 13 Feb 2016 06:45:00 PM UTC, comment #80: 

As expected, a tip (076876f0a83a) doesn't compile on Linux with e8c3590da9ff backed out.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 13 Feb 2016 06:02:20 PM UTC, comment #79: 

OK, it is this changeset by Torsten:

e8c3590da9ff "update directory in gui after add-path/change-dir dialog" (from 29 Jan 2016)

that provokes the crashes with LMB in the editor on Windows for files in the editor that are not in the search path.
Could have to do with the fact that the search path is scoured for the function in question.

From here on I'm out in the sense that I can do no more until this cset is fixed. I have no idea why it works well on Linux but crashes hard on Windows (even w/o the Windows pop-up, Octave simply disappears).

So, changes are that Lachlan's dbstop fixes work well after all .... ?

I could try a build with just this offending cset backed out. It's only bits and bytes that can break...

Philip Nienhuis <philipnienhuis>
Group Member
Fri 12 Feb 2016 09:52:24 AM UTC, comment #78: 

Well, it isn't that much work. Preparing an mxe-build is a two-step proc that takes ~5 minutes of attention; then, while the PC and its fans work hard, I can do other useful things in the mean time.
Plus all in all there aren't that many csets to take into account, maybe a handful.

I've applied all csets linearly; I backed them out also linearly but in reverse order; obviously :-)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 12 Feb 2016 06:00:07 AM UTC, comment #77: 

Thanks for all your work on this, Philip.

You mentioned earlier that with these my changesets backed out you still got a crash with the LMB.  (I assume you were referring to 65827e9cccb8 and 94fc5f13d51b applied 13 days ago).  If that is the case, then it seems that keeping trying without these changesets would be best.

If you have backed out 65827e9cccb8, then file #36310 should probably not apply.  If it crashes with file #36310 applied just after that patch, then we should probably debug that version, rather than looking for a version that doesn't crash, shouldn't we?

Lachlan Andrew <lachlan>
Thu 11 Feb 2016 07:35:04 PM UTC, comment #76: 

With your patch-4 (file #36310) applied to revision 8e317ce26a24 "unconditionally define warn_qrupdate_once" (+ a sighandlers.cc patch), Octave crashes on Windows when LMB-ing in the left margin.

OK, on to trying "221847e5f488 fix issues when restoring breakpoints and closing tabs or whole application" by Torsten

Philip Nienhuis <philipnienhuis>
Group Member
Thu 11 Feb 2016 05:24:10 PM UTC, comment #75: 

Linearly, but with big steps, only stepping changesets from Torsten and you. Once I have a hit, I can bisect back from there. I can cross-build & try 2 versions per evening ... (currently I'm at 7ebc9f38b312 "use standard message box for discarding/restoring breakpoints while saving a file"

It occurred to me that I also need to build w/o the cs of patch #8656 (I make heavily use of the Variable Editor as long as it builds in the dev tree), to be sure that it doesn't interfere. I'll try that tonight with the development tip.
Can you tell me exactly where your patch fits in with respect to the development tree? can I apply (=locally to my tree, != push to repo) it w/o fear of screwing up?

(Ah, but as you are Down Under I'll just try, if BOOM! I'll have to wait until tomorrow for your reply :-)

Philip Nienhuis <philipnienhuis>
Group Member
Thu 11 Feb 2016 09:05:04 AM UTC, comment #74: 

Philip, are you going sequentially, or using bisection to find the point at which things stop working?

Meanwhile, I've attached a fourth try at getting the left mouse button to work.  Like Torsten's recent patch, it completely reverts the octave-qt-link code.  However, this also verifies that the condition can be parsed.

I'm not sure if you want to apply a fix over the existing changeset, or back out the changeset and apply a correct one.  If you want to do the latter, I'm happy to create the replacement changeset.

(file #36310)

Lachlan Andrew <lachlan>
Tue 09 Feb 2016 09:42:45 PM UTC, comment #73: 

I started cross-building Octave versions starting with rev. ea779f11deae "strip trailing whitespace from files", adding each of the dbstop/breakpoint related csets until I find one that portrays the bug. May take a few evenings... (but I can do several other things while my PC grinds Octave in the background).

Maybe not the brightest option but I wouldn't know a better one ATM

Philip Nienhuis <philipnienhuis>
Group Member
Mon 08 Feb 2016 10:46:20 PM UTC, comment #72: 

Philip, that is puzzling.  The LMB seems to work fine under Linux, as long as my changes aren't there.  (I didn't back out the changes explicitly, but there is a particular change that I made that causes the crash, and if I undo that part, it handles the LMB.)

Doing a MXE build is still daunting for me, but I'll try to summon the courage soon :)


Torsten, we can't use Feval because, as you say, it tries to use the undefined variables.  However, we could directly call the code that Fdbstop uses for validation.  (We could even just call a dummy Fdbstop with the old line numbers, and if the line numbers change we can clear the old one and put in a new one.  However, that sounds messy.)  If you think that sounds OK, I'll prepare a patch for it.

Lachlan Andrew <lachlan>
Mon 08 Feb 2016 10:29:23 PM UTC, comment #71: 

I just tested dev Octave 4.1.0+ with both Lachlan's dbstop patches backed out (cli and gui), on Windows 7 64bit.

Guess what:
The crash still occurs with LMB in the editor.

Could it be Dan Sebald's patch [1] (applied previously) that is the culprit? (and can one/both of you confirm my observation?)

Only tomorrow or Wednesday evenings I'll have time to backout that one and build + cross-build again.

[1] (0a09c3cae800 "New marker class for modified file breakpoint and position maintenance")

Philip Nienhuis <philipnienhuis>
Group Member
Sat 06 Feb 2016 11:41:07 AM UTC, comment #70: 

Maybe we can check the syntax of the condition via Feval? I think, errors due to undefined variables can not be avoided since you can clear a variable after defining the breakpoint or the variable is not yet defined when the breakpoint is set.

Torsten Lilge <ttl>
Group Member
Sat 06 Feb 2016 10:48:19 AM UTC, comment #69: 

I should add that I'm happy with whatever solution you are happy with.

If the current default branch crashes, then I think the most important is to apply a patch that stops it crashing, and then discuss the niceties of whether/how to validate user input after that.

Lachlan Andrew <lachlan>
Sat 06 Feb 2016 10:36:37 AM UTC, comment #68: 

Yes, I had considered that sequence but wasn't sure what to do if the user enters an invalid expression.  Would the callback then send a signal to someone, containing the error message?  The discussion with Philip showed how important this validation is.

Lachlan Andrew <lachlan>
Sat 06 Feb 2016 09:57:32 AM UTC, comment #67: 

Concerning the crash and comment #64: What about using the existing callbacks for adding conditional callbacks instead of the try-construct (see attached patch)? Then we could just use the old way for the dialog box called from file_in_path. Or am I missing something?

(file #36265)

Torsten Lilge <ttl>
Group Member
Fri 05 Feb 2016 10:42:07 PM UTC, comment #66: 

It's getting weirder everyday...

I backed out bug_46795cset & dbstop_20160130_gi.cset (65827e9cccb8) and rebuilt.

On Linux it works fine when entering conditional breakpoints from the terminal, on Windows it still crashes as soon as I LMB in the editor to add a breakpoint.
On Windows, when entering conditional dbstop's from the terminal I also get error messages like "function name expected before if condition" that I do not see on Linux.
I checked twice and I'm sure it is the same source tree...

BTW, could the fix for bug #46931 have something to do with the crashes? (cs 4aa8e4b868c9)


OK, with a clean Octave repo clone, what patch to apply from the vast collection in this bug thread? (I sometimes feel a bit lost here....)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 05 Feb 2016 10:01:56 PM UTC, comment #65: 

Oops.  The previous patch had my stack-trace code.  Please consider v3.

(file #36260)

Lachlan Andrew <lachlan>
Fri 05 Feb 2016 09:48:46 PM UTC, comment #64: 

OK, I take it back about this ever having worked...

Here is a second patch for the issue of changing directories.  It still isn't perfect -- it chooses the widget to use based on whether it is called from a callback or not, rather than ensuring it is always called from the same context.  The problem with forcing it to be called non-modally is that the conditional break needs to receive a string for the condition, verify it and possibly ask for another string.  uiwidget_creator doesn't seem to have an option to read a string.

I don't really know enough about the best practice  in implementing GUIs to know what things should be done by what threads, so once it stops crashing, I might leave this to someone else (Torsten?) to design the "proper" solution.  Once it has been decided what sequence of events / threads / signals is required, I'm happy to have a go at coding it.

Philip, I tried to reproduce the error you mentioned in comment #63 but can't (under Linux).  I think it is a different issue from the one Torsten mentioned.

(file #36259)

Lachlan Andrew <lachlan>
Fri 05 Feb 2016 07:16:22 AM UTC, comment #63: 

If it may help with diagnosing:

Having breakpoints in a function file in the editor (added using "dbstop in .. at ..." statements in the terminal), then editing the file, then trying to save the file, leads to a hard crash.

However it turns out that the edits were saved after all. So I surmise the crash occurs when restoring the file and/or wiping its breakpoints in the editor.

Just for completeness: tried on Win 7, not yet verified on Linux.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 04 Feb 2016 02:33:14 PM UTC, comment #62: 

OK.  I'll have another look over the weekend.

It possibly had nothing to do with Torsten's recent patch; I may just not have tested LMB on the latest version -- it used to work, but not necessarily at the same time as the RMB did...

Lachlan Andrew <lachlan>
Wed 03 Feb 2016 08:40:20 PM UTC, comment #61: 

In addition, even with your latest patch, on Windows I get a crash when entering a breakpoint using LMB.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 03 Feb 2016 08:37:28 PM UTC, comment #60: 

Hmmm maybe better forget that "throw_if_not_natural thing", it seems to be caused by a patch for bug #33523 that I had applied for testing in the same build tree.

I'll rebuild again from scratch....

(Makes me wonder why that "throw_if_not_natural" function is called in the first place when hitting breakpoints)

Philip Nienhuis <philipnienhuis>
Group Member
Wed 03 Feb 2016 08:25:10 PM UTC, comment #59: 

Lachlan,

With and without the patch I get
"error: 'throw_if_not_natural' undefined near line XXX column YY"

They first occurred on Linux while building the docs - IIRC texinfo or makeinfo complained. I edited the out of func.texi (2 lines) and then the build continued fine.
It didn't occur to me they were caused by the dbstop patches (from Dan, you or Torsten, I don't know).

Now I also get them in Windows when Octave hits a breakpoint.

What should we do? back out the changesets (which one(s))?

Philip Nienhuis <philipnienhuis>
Group Member
Wed 03 Feb 2016 07:06:26 PM UTC, comment #58: 

With the patch, the problem remains when removing breakpoints and when running a file from the editor. And, don't we need to set the breakpoint by the worker thread, i.e. in a callback function?

Beside of that, I really do not understand what is going since the callback structures were not changed between testing and pushing your changesets. Moreover, I have backed out my recent changeset (the one that made file_in_path a non-static class member) for testing but without any success, the issue remains.

Torsten Lilge <ttl>
Group Member
Wed 03 Feb 2016 07:19:03 AM UTC, comment #57: 

achlan:
I applied that patch yesterday, also cross-built for Windows.
On Linux it works now (also the RMB stuff). I had no time for further testing yet, also for Windows.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 02 Feb 2016 09:57:57 PM UTC, comment #56: 

Yes, the problem is that the context from which the dialog is called is different for the left and right buttons.  Could either of you comment on the patch I supplied a couple of days ago (file #36224)?

The patch removes a seemingly unnecessary level of callback for the left click.  However, if you think it better instead to add an extra level of callback to the right button, I'll give that a go.

Thanks!

Lachlan Andrew <lachlan>
Tue 02 Feb 2016 06:33:01 PM UTC, comment #55: 

Ah, confirmed, Torsten. It didn't occur to me to try the simple LMB.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 01 Feb 2016 07:30:22 PM UTC, comment #54: 

Interesting, when using the right mouse click for adding a conditional breakpoint, everything works as expected. The crash occurs when using a left mouse click (ordinary breakpoint) or when running the file from the editor menu/toolbar.

Torsten Lilge <ttl>
Group Member
Mon 01 Feb 2016 07:17:11 PM UTC, comment #53: 

@Torsten:
I don't see a crash when having a function in the editor while that function is not in the path, and entering a dbstop condition by RMB-ing next to a line.
Indeed I see the pop-up then but if I add the function's directory to the path, or cd to it's directory, or press Cancel, Octave just runs on fine.

What else did you do to provoke the crash?

Philip Nienhuis <philipnienhuis>
Group Member
Mon 01 Feb 2016 06:54:14 PM UTC, comment #52: 

I have changed path_in_file from a static function into a normal member of octave_qt_link for using update_directory in it. The occurrences from where this function is called have not changed.

Torsten Lilge <ttl>
Group Member
Mon 01 Feb 2016 02:42:00 AM UTC, comment #51: 

Torsten, the problem is that I modified file_in_path to use QMessageBox, because it would hang from the context in which I called it for inserting conditional breakpoints.

However, the QMessageBox crashes when called from a callback.  Did your recent changeset add the callback for handle_request_add_breakpoint to add_breakpoint_callback?  If so, that would be the conflict.

The attached patch moves the contents of the callback into the caller, and it no longer crashes.  Do you see a problem with that approach?

It is slightly inelegant that callbacks are used by the other functions like removing a breakpoint but not for adding one.  A better long-term solution may be to fix the reason that the old file_in_path would hang.  However, elegance comes second to not crashing!

(file #36224)

Lachlan Andrew <lachlan>
Sun 31 Jan 2016 09:58:51 PM UTC, comment #50: 

Thanks for the update, Torsten.  I'm not sure if it is an interaction with your previous changeset, but it used to work.  I'll get back to you.

(And a big thanks to Philip for committing these patches.  Soon I'll get back to textscan :)

Lachlan Andrew <lachlan>
Sun 31 Jan 2016 07:27:18 PM UTC, comment #49: 

The message box appears when you try to set a breakpoint or run the file but it is not in the current search path. Then you have the options to cancel the action, change the current directory or add the file's location to the search path.

Torsten Lilge <ttl>
Group Member
Sun 31 Jan 2016 07:11:16 PM UTC, comment #48: 

Torsten, when (under what circumstances) does that message box appear?
I didn't see it when I tested yesterday before pushing.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 31 Jan 2016 12:52:45 PM UTC, comment #47: 

Lachlan, I have to reopen the bug since - at least on my system - octave crashes when the message box in octave_qt_link::do_debug_cd_or_addpath_error is shown. Can this still be an interference with my previous cset http://hg.savannah.gnu.org/hgweb/octave/rev/e8c3590da9ff ?

Torsten Lilge <ttl>
Group Member
Sat 30 Jan 2016 05:18:23 PM UTC, comment #46: 

Pushed here:
http://hg.savannah.gnu.org/hgweb/octave/rev/94fc5f13d51b (cli)
and
http://hg.savannah.gnu.org/hgweb/octave/rev/65827e9cccb8

Closing report as per comment #41

Thanks very much, Lachlan & Torsten

Philip Nienhuis <philipnienhuis>
Group Member
Fri 29 Jan 2016 11:19:05 PM UTC, comment #45: 

Thanks, Torsten.  It was an easy update, and I also added the new function in the commit message.

The new sequence is to apply
dbstop_20160124_cli.cset
then
dbstop_20160130_gui.cset

I assume they will be applied separately, in case one needs to be backed out.

(file #36201)

Lachlan Andrew <lachlan>
Fri 29 Jan 2016 10:48:43 PM UTC, comment #44: 

Sorry, this is caused by my changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/e8c3590da9ff
I am going to update the patch

Torsten Lilge <ttl>
Group Member
Fri 29 Jan 2016 06:40:57 PM UTC, comment #43: 

Didn't work out well:
+verbatim-
:
  CXX      libgui/src/m-editor/libgui_src_libgui_src_la-file-editor-tab.lo
../dev/libgui/src/m-editor/file-editor-tab.cc: In member function 'void file_editor_tab::handle_context_menu_break_condition(int)':
../dev/libgui/src/m-editor/file-editor-tab.cc:338:62: error: cannot call member function 'bool octave_qt_link::file_in_path(const string&, const string&)' without object
       || ! (octave_qt_link::file_in_path (info.file, info.dir)))
-verbatim-

Octave's development version is a rapidly moving target these days....

Philip Nienhuis <philipnienhuis>
Group Member
Fri 29 Jan 2016 05:17:08 PM UTC, comment #42: 

Oops, I meant a patch for bug #46931 (that you wanted to be applied).

Patch file #36152 dbstop_20160124_gui.cset doesn't apply cleanly with hunk #8; I think the spot is now at L.933 and the preceding if condition is changed from

if (octave_qt_link::file_in_path (info.file, info.dir))

into

if (_main_win->get_octave_qt_link ()->file_in_path (info.file, info.dir))

isn't it?

I'll make that change, build, test and check before pushing; maybe I'll consult here again.

As to patience: I think you, as author of these fixes, have expressed quite some patience/endurance as well, thanks

Philip Nienhuis <philipnienhuis>
Group Member
Fri 29 Jan 2016 03:54:58 AM UTC, comment #41: 

Philip, I think that to close this report, it is sufficient to apply
file #36151 dbstop_20160124_cli.cset  followed by
file #36152 dbstop_20160124_gui.cset

I didn't realise there is a patch for bug #46928 -- that's great.

The "...after..." patch was just to show breakpoints in a file that is opened after the dbstop command is issued.  Torsten said he was still thinking about the right way to do that, so we can leave it to him.  It is about existing dbstop behaviour anyway, not the extensions that this report is about.

Thanks for your patience with this process.

Lachlan Andrew <lachlan>
Mon 25 Jan 2016 07:44:19 PM UTC, comment #40: 

Lachlan, exactly which patches have to be pushed in what order?

FYI, a few days ago I've imported 4 debug-related patches/changesets into a fresh default tree, incl. the patch for bug #46928.
IIRC there was also a "...after..." patch.

Thanks

Philip Nienhuis <philipnienhuis>
Group Member
Sun 24 Jan 2016 12:21:32 AM UTC, comment #39: 

Great, your translations sound right to me. Now we're on the same page :)

I like your new wording for the help text.

I agree that option (1) seems the best approach for now.

Option (2) is messy because the function doesn't automatically know if it is being called by the context menu or a typed dbstop command, and I wouldn't want people to type extra quotes into the dialog box (which would turn the whole thing into a vector, giving a confusing error).  It would be a lot of code bloat for little gain, IMHO.

If you are both happy now, it would be great to have the dbstop_20160124_{cli,gui}.cset  patches applied and see if we get feedback from other people.

Thanks to you both for all your help!

(file #36151, file #36152)

Lachlan Andrew <lachlan>
Sat 23 Jan 2016 01:55:21 PM UTC, comment #38: 

@Lachlan

(please correct me if I'm wrong)
If I translate what you describe as
- "CLI" into "terminal" (or "terminal pane"),
- "GUI" into "editor pane",
and what I describe as:
- "GUI" into "any GUI pane, terminal inclusive"
- "CLI" into "octave-cli executable (!= GUI)",

... it turns out in hindsight we've been talking about the same things all along ...

I might have been a little confused here because on Windows the terminal pane (AFAIK originally based on some KDE terminal) is quite different from the octave-cli window (based on Windows' cm32.exe) and behaves quite differently in several aspects. So differences between "CLI" and "GUI" rang the wrong bell here ...

Indeed everything works as advertised. I've tried all the updated patches incl. the one from bug #46931

As to the help text:
Conditions containing quotes (", ') and comment characters (#, %) should be enclosed in extra (different) quotes if entered in the terminal. (This does not apply to conditions entered in the editor's context menu.)
Would that be OK?

As to the solutions:

- Considering that many many Octave users still prefer the CLI (no GUI), (4) and (5) seem less well applicable.

- I'd prefer (1) for the time being & consult JWE as to (3).

- As to (2), an error message along the lines of
"Did you forget double-quoting debug conditions containing quotes or comment characters?" or just
"Did you forget double-quoting debug conditions containing ", ', # or % ?",
maybe after the parser's complaints (like "warning: Error evaluating breakpoint condition:") if those can't be catched, might help?

FWIW, in my own experience error messages in my code started out fairly roughly ; only after several comments, questions and sometimes even bug reports from users and revisiting code by myself years later they got more to-the-point. That's inevitable because programmers just cannot imagine all use cases that ordinary users may come up with. So on the one hand no need to get a perfect error message right now, OTOH we just do the best we can at this moment.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 23 Jan 2016 07:55:35 AM UTC, comment #37: 

Philip, I've attached a screenshot of what I mean by "entered in the GUI".  Entered through that dialog box, the expression works without extra quotes.  I'm thinking of changing the help text from

Conditions containing quotes (", ') or comment characters (#, %) should be enclosed in quotes.  (This does not apply to conditions entered in the GUI.)

to

...in the dialog box.)

Can you suggest a better wording?


On the command line, I'm limited by what the parser permits.  If, as the help text suggests, the entire string is in quotes, then everything works:


>> dbstop in strread at 209 if 'any (format == "%f")'
ans = 209
>> dbstatus
breakpoint in strread at line 209 if any (format == "%f")


If it is not entirely in quotes, then the parser messes with things.  Look at the error message in


>> ls -l ('"%f"')
ls: cannot access ('": No such file or directory


It treats the % as a comment and passes the three-character string ('" to the command.

I can see four natural options (in decreasing order of my preference):

1. Use the current version with its limitations, relying on the documentation to get people to use quotes.  (It is easier to remember the outer quotes and get them in the right place if you think of it as quoting the whole condition, which contains a quoted %f, rather than double-quoting the %f.)

2. Catch the exception thrown by the syntax error and print something more informative.  If so, please suggest something.  Remember that we don't know that the user typed a %, and if the
% is absent, we don't know about the outer layer of quotes.

3. Get someone (jwe? Rik?) to fix the parser.

4. Only allow conditions to be entered using the dialog box.

5. Drop conditions entirely.

Which do you recommend?

FWIW, Matlab seems not lot like spaces in the condition; did you try the condition with the spaces removed?  My guess is that it would be OK.


Lachlan Andrew <lachlan>
Mon 18 Jan 2016 08:01:11 PM UTC, comment #36: 

/I wish I could look over your shoulder/
Yeah I sometimes feel like the fool that can ask more questions than 20 Wise Men can answer :-)

Looking back at comment #31 I see that I might have been a little more careful about how to express my conclusions in a least ambiguous way; apologies for that.

With "... when entering the condition in the GUI it works w/o double quotes" I meant to say "....w/o double quoting" or as you say, "two levels of quoting".  But...
turns out I messed that up too - only now I note the real issue.
In the GUI:

>> dbclear strread
>> dbstop in strread at 209 if format(2) == "f"
ans =  209
>> strread ("1 2 3", "%s")
warning: Error evaluating breakpoint condition:
    'f' undefined near line 1 column 14
stopped in /home/philip/devel/octdev/oct410+VExTSdbs/../dev_VExTS/scripts/io/strread.m at line 209
209:   if (strcmp (typeinfo (format), "sq_string"))
debug> format(2)
ans = s
debug> dbquit

>> dbclear strread
>> dbstop in strread at 209 if format(2) == '"f"'
ans =  209
>> strread ("1 2 3", "%s")
ans =
{
  [1,1] = 1
  [2,1] = 2
  [3,1] = 3
}
>>


...which shows that in the GUI double-quoting is equally necessary as in the CLI to avoid misinterpreting/mis-parsing the "%": 
W/o double-quoting the breakpoint is triggered but not because the "if" condition is met, rather because evaluating the "if" condition errors out behind the curtains (i.e., in the parser?).
That is surely something that unwary users will overlook and may provoke questions in the ML/bug reports.

I also see this:

>> dbstop in strread at 209 if strcmp (format, '"%f"')
parse error:

  syntax error

>>> strcmp (format, '" ;
                       ^


Again parser issues, I suppose.

FWIW, Matlab doesn't accept the above dbstop construct at all, so maybe we shouldn't bother too much.

As to the first verbatim block, I can't reproduce it now either, I also get the warnings.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 18 Jan 2016 10:28:47 AM UTC, comment #35: 

Oops.  I forgot to qref before exporting.  The correct gui changeset is dbstop_20160118_gui_again.cset


(file #36077)

Lachlan Andrew <lachlan>
Mon 18 Jan 2016 05:05:48 AM UTC, comment #34: 

Torsten, is there a reason to use eval_string?  Is it modularity? Since the gui is part of Octave, it seems to me that calling Octave internals directly is the logical approach.

Lachlan Andrew <lachlan>
Mon 18 Jan 2016 05:02:30 AM UTC, comment #33: 

Thanks again for your testing Philip.

I've updated the GUI patch to suppress the message on the console.

I got a bit confused when you spoke of double vs single quoting.  I assume you mean two vs one levels of quoting, rather than " vs '.  In that case, I'm not sure why the first example you gave didn't cause it to break.  I always get the second "verbatim" block.  I wish I could look over your shoulder as you do this :)

Unless I change the parser, there is nothing I can do to detect the quotes in "f".  They're removed before dbstop is called.  However, I have updated the documentation with an example (your favourite  dbstop in strread at 209 if 'any (format == "%f")') and put quotes in the example usage line.  It now also applies cleanly to the tip of the default branch.

In regard to your question '/typing "dbstop ...", not necessarily in the octave-cli executable/
Where else should I enter this dbstop .... command?'  the place other than the  octave-cli  executable is the command window of the  octave-gui  executable.

(file #36072, file #36073)

Lachlan Andrew <lachlan>
Sun 17 Jan 2016 06:13:39 PM UTC, comment #32: 

Just to clarify, my dev tree was at d6fa72047aeb "Update documentation of "exist" to warn of buggy .oct files (bug #36067)"
and I applied the patches 36040, 36034 & 36041 (in that order; I had to polish after hunk #10 of the first).

Philip Nienhuis <philipnienhuis>
Group Member
Sun 17 Jan 2016 06:06:50 PM UTC, comment #31: 

Trying the new series of patches (3 pcs), I get:

>> dbstop in strread at 209 if format(2) == "f"
ans =  209
>> strread ("1 2 3", "%f")
warning: Error evaluating breakpoint condition:
    'f' undefined near line 1 column 14
ans =

   1
   2
   3
>> dbclear strread
>> dbstop in strread at 209 if format(2) == '"f"'
ans =  209
>> strread ("1 2 3", "%f")
debug>


so things seem to work well. The double-quoting around %f required in the cli might be a bit of a nuissance (I fear that people will overlook or forget this).

With single quotes I get slightly different messages:

>> strread ("1 2 3", "%f")
warning: Error evaluating breakpoint condition:
    'f' undefined near line 1 column 14
stopped in /home/philip/devel/octdev/oct410+VExTSdbs/../dev_VExTS/scripts/io/strread.m at line 209
209:   if (strcmp (typeinfo (format), "sq_string"))
debug>


And when entering the condition in the GUI it works w/o double quotes.
In the GUI I also see this: when I right-click and enter a wrong condition (assignment rather than comparison) I see two error messages: one in the gui, and
"error: dbstop: condition cannot be an assignment.  Did you mean '=='?"
in the terminal pane.
I think the latter can be dropped if one is working in the editor pane. But of course this is just a bit nitpicking.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 15 Jan 2016 12:07:21 PM UTC, comment #30: 

@Lachlan, comment #27:

Oopsie, you're absolutely right about the string comparison.

(below still with your big all-in-one patch, haven't had an opportunity to build with tip & newer patches)
Right-clicking in the editor margin and entering
strcmp (format, "%f")
in the dialog box emits the same error in the terminal:

>> dbstop in strread at 209 if strcmp (format, "%f")
error: unterminated character string constant
>>


Right clicking in the margin & entering 'format(2) == "f"' (w/o single quotes) is acceptable (see below) but again yields some error:

>> dbstop in strread at 209 if format(2) == "f"
ans =  209
>> strread ("1 2 3", "%f")
warning: Error evaluating breakpoint condition:
    'f' undefined near line 1 column 14
stopped in C:\Programs\Octave\Octave-4.1.0+_w64VExTS\share\octave\4.1.0+\m\io\strread.m at line 209
209:   if (strcmp (typeinfo (format), "sq_string"))
debug> format
format = %f
debug> format(2) == 'f'
ans =  1
debug>



While not very experienced with debugging invoking conditional breakpoints, I'd expect average users to believe that a "dbstop if ... " should accept any if condition valid in the context of the function to-be-debugged; even when introducing new "constants" or "variables" like the 'f' in the example above.
And yes, that would include (user-specified !) any() and all() as an if condition. But no, dbstop shouldn't substitute those.

As to the CLI, I'm still missing something:
/typing "dbstop ...", not necessarily in the octave-cli executable/
Where else should I enter this dbstop .... command?

Apparently I'm still confused :-)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 15 Jan 2016 06:07:55 AM UTC, comment #29: 

Sorry, I forgot to mention the color change. The yellow position pointer was hard to recognize within the yellow circle, that's why I made it more orange.

Concerning the dbstatus, I was thinking about a solution. Dan's patch is relying on the function eval_string for executing commands. Playing around with this function, I recognized a slightly different behaviour of some commands (I remenber cd ()).

Torsten Lilge <ttl>
Group Member
Fri 15 Jan 2016 05:09:58 AM UTC, comment #28: 

I've briefly tested the new patch, Torsten, and it seems to work as intended.

You changed the colour of the conditional breakpoint marker.  Was there a particular reason?  The reason I ask is that I'm thinking of making a new marker class to mark the current line in the stack frame entered by "dbup" or "dbdown".  I was thinking of making it either dark grey or greyish green.


The testing revealed an existing limitation: if there is a breakpoint in a file, then opening the file doesn't load the breakpoints into the editor.  Dan Sebald's original patch relied on patch #8016, which would allow the gui to call "dbstatus" using signals.  Instead, I have written some code to query the db_table data structure directly.

Since that new code isn't specific to the expanded dbstop, I've made a separate patch [dbstop_20160115_after.cset], to be applied after your dbstop_20160114_gui.patch

(file #36041)

Lachlan Andrew <lachlan>
Fri 15 Jan 2016 03:35:38 AM UTC, comment #27: 

Torsten, thanks for pushing the patch.  I'll test the new gui patch soon.

Philip, first, when I said "the gui", I meant right-clicking on the bookmark margin and entering a condition into the dialog box, and by "the CLI", I meant typing "dbstop ...", not necessarily in the octave-cli executable.  Given that, do my previous comments make sense?

The warning you get is a warning issued by the patch, not about the patch. It is because  format  is an array, and so 'format == "%f"' returns an bool matrix and not scalar boolean.

I've updated the patch to give a clearer warning for that, since it is likely to be common.  Alternatives would be to use all() [roughly what "if" does, but not quite] or any(), but I think both of those are likely to lead to confusion and so would prefer to keep it as a warning.  What do you think?

(file #36040)

Lachlan Andrew <lachlan>
Thu 14 Jan 2016 07:32:59 PM UTC, comment #26: 

After cset
http://hg.savannah.gnu.org/hgweb/octave/rev/221847e5f488
Lachlan's cli-patch (file #36007) can still be applied as it is. Please find attached the updated gui-patch for further testing.

(file #36034)

Torsten Lilge <ttl>
Group Member
Wed 13 Jan 2016 08:34:55 PM UTC, comment #25: 

The marker patch is pushed with cset
http://hg.savannah.gnu.org/hgweb/octave/rev/759fcdf3666d

Torsten Lilge <ttl>
Group Member
Wed 13 Jan 2016 06:44:28 PM UTC, comment #24: 

I have pushed the patch mentioned in comment #20 with cset
http://hg.savannah.gnu.org/hgweb/octave/rev/221847e5f488
and am going to update Lachlan's patches within the next days.

Torsten Lilge <ttl>
Group Member
Wed 13 Jan 2016 07:37:40 AM UTC, comment #23: 

@Lachlan:
I'm a bit confused as it's in the GUI that I hit those "%" issues (I rarely if ever use the CLI these days).

Both in GUI and CLI, using either form you suggested ("doubly quoted" or just "'%f"') I note other error messages (with the previous all-in-one patch):

>> dbstop ("in", "strread.m", "at", "209", "if", 'format', '==', '"%f"')
ans =  209
>> strread ( "1 2 3", "%s")
warning: Error evaluating breakpoint condition:
    octave_base_value::bool_value(): wrong type argument 'bool matrix'
stopped in C:\Programs\Octave\Octave-4.1.0+_w64VExTS\share\octave-4.1.0+\m\io\strread.m at line 209
209:   if (strcmp (typeinfo (format), "sq_string"))
debug> dbcont
ans =
{
  [1,1] = 1
  [2,1] = 2
  [3,1] = 3
}
>>


Intriguingly, the first time I ran this in the GUI it all worked beautifully and I didn't see these error messages, they turned up only after I ran it in the CLI.

I'll see if I can try the new patches this week.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 13 Jan 2016 06:41:18 AM UTC, comment #22: 

Thanks for the feedback in comment #18, Philip. I hadn't tested conditions with quotes before.  The problem is with the parser, before it gets to the dbstop command at all.  For example,


dbstop in strread at 209 if format == '"%f"'


works fine, as does


dbstop ("in", "strread.m", "at", "209", "if", 'format', '==', '"%f"')


Somehow, the parser is treating the % as a comment character, even though it is in quotes.

For the CLI, I think the best option is simply to document the fact that "special" characters in the condition need to be in quotes.  I've improved the validation of the condition so it should sometimes give clearer messages.  The GUI works fine, since it bypasses the parser.


The three changesets are attached, to be applied in the order markers, cli, gui.

(file #36007, file #36008, file #36009)

Lachlan Andrew <lachlan>
Sat 09 Jan 2016 03:42:19 PM UTC, comment #21: 

Thanks Torsten,

the editor's behavior when debugging is much better now :-)

Philip Nienhuis <philipnienhuis>
Group Member
Sat 09 Jan 2016 02:38:54 PM UTC, comment #20: 

I have prepared a patch fixing the problem with restoring breakpoints when saving a file and saving files when closing tabs or the whole application. Since there are conflicts with Lachlan's patch with work still in progress, I propose to wait for Lachlan's separated patches before pushing my patch and updating Lachlan's patches afterwards.

Torsten Lilge <ttl>
Group Member
Sat 09 Jan 2016 02:26:15 PM UTC, comment #19: 

Philip, you are correct. Changeset
http://hg.savannah.gnu.org/hgweb/octave/rev/5c5af828c248
fixes the scrolling for the instruction pointer, too.

Torsten Lilge <ttl>
Group Member
Sat 09 Jan 2016 01:25:24 PM UTC, comment #18: 

I just tried Lachlan's latest (20160108) (big all in one) patch.

I get:

>> dbstop in strread.m at 209 if (format == " ")
ans =  209
>> dbstop in strread.m at 209 if (format == "%f")
error: unterminated character string constant
>> dbstop in strread.m at 209 if (format == "f")
ans =  209
>> dbstop in strread.m at 209 if (format == '%d')
error: unterminated character string constant


Apparently "%" is offending in some way.

Another observation: I can only enter conditional breakpoints if the condition is enclosed between parentheses. No big dealbut is this intended?

@Torsten:
As to scrolling: jumping / scrolling the new breakpoint line to the vertical center of the editor pane doesn't happen anymore when setting breakpoints. Fine, thanks.
However, the instruction pointer is still fixed at the vertical center. I mean, that when dbstep-ing through a debugged function using the step button in the editor menu, the current line is always in the middle. Can that behavior also be changed, please?

Philip Nienhuis <philipnienhuis>
Group Member
Fri 08 Jan 2016 10:33:41 PM UTC, comment #17: 

The GUI issues were the ones I had mentioned (clicking on a yellow marker didn't clear it, and "find next" only found the next if there is an unconditional breakpoint after it) and the fact that having an unconditional breakpoint marker stopped the creation of a conditional one on the same line, and vice versa.

If/when you both are happy with the basic functionality of the patch, I think it would be best to split it into three:
0. Fix the trivial issues with Dan's code
1. Implement the new dbstop options
2. Integrate the new dbstop into the GUI.

(In hindsight, I should have done that at the start, but I accidentally merged 2 into 1 and was too impatient to split them again ):  I'll also submit changesets instead of patches, once you are happy to apply them.

Lachlan Andrew <lachlan>
Fri 08 Jan 2016 07:00:51 PM UTC, comment #16: 

Lachlan, thanks for the new version of the patch. What GUI issues are you exactly addressing in the patch? Could these fixes be separated from the (already very long) patch?

Torsten Lilge <ttl>
Group Member
Fri 08 Jan 2016 09:56:34 AM UTC, comment #15: 

Here is the updated patch that fixes some GUI issues.  Torsten, thanks for removing the restore-breakpoint dialog -- if I had done it, I would have forgotten the .ui file.

I've also uploaded a draft commit message.  It's rather long, but the patch touches lots of functions.

I should have mentioned that the patch also fixes some minor bugs with Dan's marker object, such as several instances of


-          if (markers_mask && (1 << marker::bookmark))
+          if (markers_mask & (1 << marker::bookmark))


and a few lines like


-  if (_edit_area->markersAtLine (line) && (1 << marker::bookmark))
-    line++; // we have a breakpoint here, so start search from next line
+  line++; // Find bookmark strictly after this line.
-verbatim

Should that be a separate patch to keep things clean, or is it OK to lump them with this one?

Philip, regarding comment #9 -- I don't mind which of you does the final pushing, but it is great having feedback and testing from both of you.  I removed the example usage "dbstop in func if cond", as you suggested.  Also, I can imagine that converting Octave code to Matlab would feel like downgrading; I always try to keep my code Matlab compatible because I use that at work and Octave on my notebook, and it is frustrating not being able to += etc.

(file #35944, file #35945)

Lachlan Andrew <lachlan>
Fri 08 Jan 2016 02:09:01 AM UTC, comment #14: 

Torsten, you have been productive!  The reopening of the tab would explain why it seemed to me that the tab didn't close when breakpoints were present.

I haven't got beyond familiarising myself with the code, so your patches won't interfere at all :)  This evening, I'll send a patch to remove the restore-breakpoint dialog, and a separate revised dbstop patch.

Lachlan Andrew <lachlan>
Thu 07 Jan 2016 07:14:09 PM UTC, comment #13: 

I forgot to mention that even removing the restore-breakpoint dialog (which seems to be reasonable following our discussion) the problem remains: When the worker thread needs more time to restore the breakpoints than the gui thread for removing the tab, the tab will be reopened.

Torsten Lilge <ttl>
Group Member
Thu 07 Jan 2016 07:04:16 PM UTC, comment #12: 

Lachlan, you are right. the problem is that the user should be able to cancel exiting the application and therefore a tab should not be removed immediately after saving. I am already working on a patch that uses a second flag "restore_breakpoints" which is true except in the case when called from "handle_file_modified_answer".

I have also prepared patches for

  • resetting the read-only state of some tabs when cancelling exiting the application (I found that bug while looking into the restore-breakpoint issue)
  • not centering a line after a breakpoint was added (except the line is exactly at the top or the bottom of all visible lines.


I hope this does not interfere with your current efforts.

Torsten Lilge <ttl>
Group Member
Thu 07 Jan 2016 11:08:21 AM UTC, comment #11: 

The reason that the restore breakpoints dialog is activated is in the code libgui/src/m-editor/file-editor-tab.cc


void
file_editor_tab::handle_file_modified_answer (int decision)
{
  if (decision == QMessageBox::Save)
    {
      // Save file, but do not remove from editor.
      save_file (_file_name, false);
    }


This shows that the save_file code is called believing that the tab will stay around.

This is called from the function with the following comment:


file_editor_tab::check_file_modified ()
{
  int decision = QMessageBox::Yes;
  if (_edit_area->isModified ())
    {
      // File is modified but not saved, ask user what to do.  The file
      // editor tab can't be made parent because it may be deleted depending
      // upon the response.  Instead, change the _edit_area to read only.
      QMessageBox::StandardButtons buttons = QMessageBox::Save |
                                             QMessageBox::Discard |
                                             QMessageBox::Cancel;


which suggests that the dialog should call save_file with the "close after saving" option.  However, I suspect that will cause problems with the signal emitted by


file_editor_tab::closeEvent (QCloseEvent *e)
{
  _cancelled = false;  // prevent unwanted interaction of previous
                       // exits of octave which were canceled by the user

  if (check_file_modified () == QMessageBox::Cancel)
    { // ignore close event if file is not saved and user cancels
      // closing this window
      e->ignore ();
    }
  else
    {
      e->accept ();
      emit tab_remove_request ();
    }
}


Perhaps the solution is to call the save_file with the close-after-save option and check for three possible values of check_file_modified(), instead of just checking whether or not it was "cancel".

Does that sound OK, Torsten?  If so, I'll create a separate patch for that.

Lachlan Andrew <lachlan>
Thu 07 Jan 2016 09:54:52 AM UTC, comment #10: 

Thanks for the discussions.  I've found a couple of bugs in the gui part (clicking on a yellow marker doesn't clear it, and "find next" only finds the next if there is an unconditional breakpoint after it).

I'll post a revised patch, but want to track down the save-on-close issue first.  (Torsten, you said you'd look into it; let me know if I'd be treading on your toes...)

Lachlan Andrew <lachlan>
Wed 06 Jan 2016 08:00:56 PM UTC, comment #9: 

@Torsten:
(saving/restoring breakpoints) In this case I think it does make sense to drop the dialog: I fully agree with last stanza of comment #3.
But it is up to the one who is finally patching & pushing... (I am a bit confused about who is going to do what here)

my .02$c

Thank you

Philip Nienhuis <philipnienhuis>
Group Member
Wed 06 Jan 2016 06:51:04 PM UTC, comment #8: 

@Philip: If we decide that there are no reasons for not restoring breakpoints after saving a file, I have no objections to remove the dialog and the related preferences.

@Lachlan: The issue when closing a modified file is that the breakpoint-restore-dialog appears although the file will be closed afterwards. Closing an unmodified file with breakpoints (or bookmarks) does not invoke any dialog.

Torsten Lilge <ttl>
Group Member
Wed 06 Jan 2016 03:59:05 PM UTC, comment #7: 

Automatically restoring breakpoints is also what Matlab does - I just had a day of adapting Octave code to Matlab (OT: felt like downgrading) so I had a good peek at things :-)

Philip Nienhuis <philipnienhuis>
Group Member
Wed 06 Jan 2016 11:32:05 AM UTC, comment #6: 

@Torsten,
I think the issue of prompting for saving when closing the tab is a deeper issue of not actually closing the window if there are breakpoints (not sure about bookmarks).

Also, what do you think about automatically restoring breakpoints without prompting?  I'd prefer to keep code "light" by eliminating the option of not restoring them.

Lachlan Andrew <lachlan>
Wed 06 Jan 2016 10:59:11 AM UTC, comment #5: 

@Torsten:
As to the unintended scrolling, could you also make it so that file contents only scroll when the instruction pointer gets too near to the bottom/top of the editor pane?
FWIW, that is also the behavior of Matlab's editor.

(I suppose my wish stems from the fact that I always have the editor pane undocked and at full screen height, while other people (Matlab users at work, the occasional screen shots here in the Octave mailing lists) have the editor at about half screen height docked above/below the terminal pane. In the latter case, keeping the pointer vertically centered may be more helpful.)

@Lachlan:
Again, Matlab doesn't behave any better, but still again, I may (do?) not fully understand how that dbstop option is to be used in practice.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 06 Jan 2016 06:08:02 AM UTC, comment #4: 

I think the unintended scrolling is a side effect of
http://hg.savannah.gnu.org/hgweb/octave/rev/aab7a3c7168e

The behavior for restoring the breakpoints can be adjusted in the settings or by checking "don't ask me again" in the dialog box. This was introduced with
http://hg.savannah.gnu.org/hgweb/octave/rev/0a09c3cae800
However, the dialog should not appear when the file is closing.

I will have a look at both issues.

Torsten Lilge <ttl>
Group Member
Wed 06 Jan 2016 01:49:07 AM UTC, comment #3: 

Thanks for your helpful feedback, Philip.

You are right that "dbstop in ... if ..." is not very useful without an "at <line>".  I was toying with the idea of allowing the condition to be evaluated in the caller's scope instead of the function's scope, perhaps with a syntax like


dbstop in <function> if ../(iter==3)


(Mixing variables from different scopes may be nice, but would mess with the parser too much.) However, I was hoping to have a patch that fixes compatibility separately from one that extends functionality.  Also on my list of extensions is "dbstop once", which would clear the breakpoint as soon as it has been triggered, to allow "stepping" through the code in big chunks, such as stepping over a loop.

I agree that the scrolling is annoying.  It is the behaviour of existing code, so I'll try to create a separate patch for that, though I have no idea how yet.  I don't think it needs a separate bug report, but that's your call.

The issue of asking to restore breakpoints is again someone else's code.  My preference would be always to save and restore breakpoints, and never ask.  That will save a little code bloat.  Can you think of any reason someone would not want to restore the breakpoints, given they can just click a button to clear all breakpoints anyway?  If so, I'll just make save-without-asking the default an keep the code to allow it to be changed.

Lachlan Andrew <lachlan>
Tue 05 Jan 2016 10:01:33 PM UTC, comment #2: 

Another little corner case:

When debugging a file, editing it w/o saving, and closing Octave, the editor asks: "Save Discard Cancel?" (= OK); but when clicking "Save" it still asks for restoring the breakpoints - which is obviously moot as Octave is closing down :-)

Philip Nienhuis <philipnienhuis>
Group Member
Sun 03 Jan 2016 06:35:18 PM UTC, comment #1: 

Thanks for this patch; I have just played a little bit with it.

While I am not very familiar with debug, most of the new functionality works fine AFAICS.
I'm not aware of any scoping rules for dbstop like e.g., debug_on_error has ("local" argument) but I've never experimented with it.

I tried the form "dbstop in <function> if (condition)". That form only seems to work if the condition can be evaluated at the first statement of the function, i.e., if the variable referenced in (condition) already exists upon function entry. That can only be done for "variables" nargin and nargout.
It would be easier to understand for users if the evaluator can wait until the variable comes into existence. But undoubtedly I am expecting too much :-)  Matlab behaves the same after all.


A little annoying is that when clicking in the left column to set a breakpoint, the editor contents scroll up or down so that the new breakpoint is vertically "centered".

This new behavior resembles a little gripe of me with the current editor behavior: when stepping through a debugged m-function, the current line is always exactly vertically centered.
I rather wish the file contents would stay in place as much as possible and only scroll up when the "current line" (or "instruction pointer" if you wish) gets to be 3 or 4 lines above the bottom (above editor status bar) and when jumping backwards (while/for/do-until loops) scrolls down so that the current line is 3-4 lines below the top (below editor menu bar).

Should I enter a separate bug report on this, or can it be mended while you are at it?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 03 Jan 2016 06:58:50 AM UTC, original submission:  

The following Matlab commands give errors in Octave:


dbstop in ls
dbstop if error
dbstop ls 1 if i==1
s=dbstatus; dbclear all; dbstop (s)
dbstop if caught error


The first two can be achieved with different syntax, but the other three are missing functionality.

The attached patch overhauls dbstop to make it almost compatible with Matlab, including the integration of conditional breakpoints into the GUI.

Ideally, it would be applied as a series of smaller changesets, but I got no feedback when I submitted the smaller patch #8784 and it is hard keeping too many patches up-to-date with all of the changes happening to the global code.

Please let me know what I can do to get this patch applied.

Lachlan Andrew <lachlan>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #36392:  bug_46795_v5.patch added by ttl (8KiB - text/x-diff)
file #36336:  backout_e8c3590da9ff_v2.cset added by ttl (8KiB - application/octet-stream)
file #36332:  file-editor-tab.cc added by lachlan (76KiB - text/x-c++src)
file #36328:  backout_e8c3590da9ff.cset added by lachlan (8KiB - application/octet-stream)
file #36329:  bug_46795_debugging.cset added by lachlan (2KiB - application/octet-stream)
file #36310:  bug_46795_v4.cset added by lachlan (8KiB - application/octet-stream)
file #36265:  bug46795_file_in_path.diff added by ttl (5KiB - text/x-diff)
file #36260:  bug_46795_v3.cset added by lachlan (8KiB - application/octet-stream)
file #36259:  bug_46795_v2.cset added by lachlan (9KiB - application/octet-stream)
file #36224:  bug_46795.cset added by lachlan (2KiB - application/octet-stream)
file #36201:  dbstop_20160130_gui.cset added by lachlan (45KiB - application/octet-stream)
file #36151:  dbstop_20160124_cli.cset added by lachlan (72KiB - application/octet-stream)
file #36152:  dbstop_20160124_gui.cset added by lachlan (44KiB - application/octet-stream)
file #36140:  dbstop_dialog.png added by lachlan (170KiB - image/png)
file #36077:  dbstop_20160118_gui_again.cset added by lachlan (45KiB - application/octet-stream)
file #36072:  dbstop_20160118_cli.cset added by lachlan (72KiB - application/octet-stream)
file #36073:  dbstop_20160118_gui.cset added by lachlan (43KiB - application/octet-stream)
file #36041:  dbstop_20160115_after.cset added by lachlan (4KiB - application/octet-stream)
file #36040:  dbstop_20160115_cli.cset added by lachlan (73KiB - application/octet-stream)
file #36034:  dbstop_20160114_gui.patch added by ttl (45KiB - text/x-diff)
file #36007:  dbstop_20160113_cli.cset added by lachlan (72KiB - application/octet-stream)
file #36008:  dbstop_20160113_markers.cset added by lachlan (9KiB - application/octet-stream)
file #36009:  dbstop_20160113_gui.cset added by lachlan (44KiB - application/octet-stream)
file #35944:  dbstop_patch_20160108.txt added by lachlan (118KiB - text/plain)
file #35905:  dbstop_patch_20160102.txt added by lachlan (108KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by lachlan (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 25 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-02-19 philipnienhuis StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2016-02-18 ttl Attached File- Added backout_e8c3590da9ff_v3.patch, #36391
        Attached File- Added bug_46795_v5.patch, #36392
    2016-02-14 ttl Attached File- Added backout_e8c3590da9ff_v2.cset, #36336
    2016-02-14 lachlan Attached File- Added file-editor-tab.cc, #36332
    2016-02-14 lachlan Attached File- Added backout_e8c3590da9ff.cset, #36328
        Attached File- Added bug_46795_debugging.cset, #36329
    2016-02-11 lachlan Attached File- Added bug_46795_v4.cset, #36310
    2016-02-06 siko1056 Carbon-CopyRemoved 91577 -
    2016-02-06 ttl Attached File- Added bug46795_file_in_path.diff, #36265
    2016-02-05 lachlan Attached File- Added bug_46795_v3.cset, #36260
    2016-02-05 lachlan Attached File- Added bug_46795_v2.cset, #36259
    2016-02-01 lachlan Attached File- Added bug_46795.cset, #36224
    2016-01-31 ttl StatusFixed In Progress
        Open/ClosedClosed Open
    2016-01-30 philipnienhuis StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-01-29 lachlan Attached File- Added dbstop_20160130_gui.cset, #36201
    2016-01-24 lachlan Attached File- Added dbstop_20160124_cli.cset, #36151
        Attached File- Added dbstop_20160124_gui.cset, #36152
    2016-01-23 lachlan Attached File- Added dbstop_dialog.png, #36140
    2016-01-18 lachlan Attached File- Added dbstop_20160118_gui_again.cset, #36077
    2016-01-18 lachlan Attached File- Added dbstop_20160118_cli.cset, #36072
        Attached File- Added dbstop_20160118_gui.cset, #36073

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code