patchGNU Octave - Patches: patch #8006, New marker class to maintain...

 
 

patch #8006: New marker class to maintain breakpoints and position in modified files

Submitter:  Dan Sebald <sebald>
Submitted:  Mon 08 Apr 2013 09:36:43 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  ttl Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 29 Dec 2015 06:56:16 PM UTC, comment #17: 

@Torsten: Your commit message mentions marker.cc and marker.h but they are missing from the cset.

This causes the build to fail, see bug #46779

Andreas Weber <andy1978>
Group Member
Tue 29 Dec 2015 05:26:15 PM UTC, comment #16: 

I have updated and pushed the patch with cset
http://hg.savannah.gnu.org/hgweb/octave/rev/0a09c3cae800

Following updates were necessary:

  • No icons for the marker, since only QsciScintilla internal markers are used in the gui
  • Removed the settings for the "unsure icon set" (it is just a grey marker)
  • commented out some code which is not yet used because of missing possibility to return results from octave to the gui


Moreover, before adding a new position marker, all exisitng positions are removed in order to avoid "zombie" position markers

Torsten Lilge <ttl>
Group Member
Tue 29 Dec 2015 09:24:06 AM UTC, comment #15: 

Is there any chance of this patch being updated again?  I'd love to use it, but don't know Qt well enough to apply it to the current version.

Core developers: is there a reason for not applying this patch to default?

Lachlan Andrew <lachlan>
Sat 07 Feb 2015 05:13:12 PM UTC, comment #14: 

OK, I see.  Over a year ago already.

Dan Sebald <sebald>
Sat 07 Feb 2015 11:40:21 AM UTC, comment #13: 

Concerning the mentioned code lines I have updated the patch because changeset http://hg.savannah.gnu.org/hgweb/octave/rev/f26d527c1a71 removed the dir and function_name from the initialization of bp_info, i.e.


file-editor-tab.cc:
-  bp_info info (_file_name, dir, function_name, line+1);
+  bp_info info (_file_name, line+1);

file-editor-tab.h:
-    bp_info (const QString& f, const QString& d, const QString& fn, int l)
-      : file (f.toStdString ()), dir (d.toStdString ()),
-        function_name (fn.toStdString ()), line (l)
-    { }
+    bp_info (const QString& fname, int l = 0);


Torsten Lilge <ttl>
Group Member
Fri 06 Feb 2015 08:13:28 PM UTC, comment #12: 

Sorry, I didn't see your post until now.

I tried your updated patch and fixed one rejected hunk in libgui/src/m-editor/file-editor-tab.h  I tested on a function by moving some breakpoints and then saving.  The behavior is still as designed.  I see the added window inquiring if breakpoints should be saved.

The only question I have (comparing the two diff files in Meld) is that you found no need for the directory and function name when setting the breakpoint, i.e.,


-  bp_info info (_file_name, line+1);
+  bp_info info (_file_name, line);


as opposed to


-  bp_info info (_file_name, dir, function_name, line+1);
+  bp_info info (_file_name, dir, function_name, line);


Is that extra information needed to resolve files of the same name?  Or perhaps _file_name is now full path file name?  It's been a while, and I'm trying to jog my memory.

Dan Sebald <sebald>
Wed 31 Dec 2014 12:56:02 PM UTC, comment #11: 

Dan, I came across your patch while looking at bug #42708. Attached please find the patch updated for the current gui-release branch. Could you please test if the functionality of the intended changes is okay?


(file #32739)

Torsten Lilge <ttl>
Group Member
Sat 11 May 2013 01:30:38 AM UTC, comment #10: 

BTW, on the topic of QScintilla and deleting a line that contains a marker, I did hear back from some folks on the QScintilla discussion list.  That would be #3 in the original post here.  This topic has come up before, but it sounds like wouldn't be much motivation to implement something from the QScintilla or Scintilla group.  Someone on the list gave these two links:

https://groups.google.com/forum/#!msg/scintilla-interest/48WKhPHsH6M/5EClWdRNVQ8J

http://www.scintilla.org/ScintillaDoc.html#Markers

The first link is a discussion of the markers and an argument against storing such information with the Undo text, which is that for example someone wouldn't want breakpoints restored as part of restoring a number of changes.  In my opinion that's a marginal argument.  However, accepting that, I'm not sure I agree with the idea of ORing the remaining markers with the previous line's marker.  Someone would want their breakpoint to be associated with the line before or after?

Anyway, the second link describes how to get marker information as part of a "notification" (scintilla parlance for sending information to an app about what is happening in the editor).  In particular, the symbol SC_MOD_BEFOREDELETE is the one of interest.  It was suggested to write one's own "markerLineDeleted" type of routine.  So, that would be a little involved in the sense one would create a class derived from QScintilla inheriting all the functions and add one or two additional routines, routines that use the Scintilla notification code.  Eh, not sure I want to look into that right now.  Just live with Scintilla's behavior for now, save any such mods for a future changeset.

Dan Sebald <sebald>
Fri 10 May 2013 06:55:50 PM UTC, comment #9: 

New version of changeset as described in comment #8, i.e., removed line of code from remove_all_breakpoints_callback that was clearing all markers (which wasn't correct anyway).  The debugger core now removes breakpoints inside local functions, which consequently removes the markers as well.

(file #28064)

Dan Sebald <sebald>
Wed 08 May 2013 06:46:39 AM UTC, comment #8: 

This changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/de91b1621260

appears to have solved the problem of uncleared breakpoints inside local functions when a remove_all_breakpoints is done.

The hunk in (file #28020) associated with the verbose comment about cleaning up breakpoints in local functions can be removed.  I'll create a new changeset if needed.

Dan Sebald <sebald>
Sun 05 May 2013 02:15:17 AM UTC, comment #7: 

Thanks for the --exact option information.  I now understand things a little more than I used to:

http://mercurial.selenic.com/wiki/RebaseExtension

and realize why that fftfilt patch was a mess.  Not sure rebase is any easier than working from the diff hunks.  Rebase will launch an editor to resolve conflicts, but it also includes all the mods that have taken place since the time of the original patch which became quite lengthy.  I'm probably still not using it correctly.

In any case, I've attached a new patch that has all the PNG files and so on as part of the changeset.  The behavior is working pretty nicely with improved breakpoint clear and reinstating when saving a file.  Still might be an issue with clearing breakpoints in local functions in the core, but those local function breakpoints do work now.

(file #28020)

Dan Sebald <sebald>
Wed 01 May 2013 05:00:08 PM UTC, comment #6: 

If you haven't been throwing out your changes, you can use "hg import --exact the_patch" to apply the patch at the exact commit where it was created (so that there can be no failed hunks). Then you can modify the patch with "hg commit --amend" to fix the patch and/or "hg rebase" to base the patch at a new commit. The latter uses merge logic, so you can use a merge tool instead of having to much with failed hunks.

Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Wed 01 May 2013 05:47:15 AM UTC, comment #5: 

Oo, lot's of failed hunks, and not enough time to finish tonight.  I'll look into this on May 3.

Dan Sebald <sebald>
Tue 30 Apr 2013 08:43:21 PM UTC, comment #4: 

I'll create a changeset with the changes you suggested later this evening.

Dan Sebald <sebald>
Tue 30 Apr 2013 08:35:45 PM UTC, comment #3: 

Can you create a new version of the patch that removes the debugging statements and fixes the copyright statements, or do you want me to do that?

For the binary files, try adding


[diff]
git = 1


to your hgrc file.  I think that is also required to make exported changesets properly track hg renamed files.

John W. Eaton <jwe>
Group administrator
Tue 30 Apr 2013 08:15:14 PM UTC, comment #2: 

Yes, the fprintf are only temporary things.  (I use them because then it is easy to search and remove.)  In this case they are just meant to point out it is in fact the core debugger that is creating the placing the debug marker in the subfunction rather than some bad GUI code.  Hard to tell sometimes.  (Hopefully that is fixed with the changeset you pointed out.)

Oddly enough, I couldn't figure out how to include the PNG files in the changeset.  The files were added to the mercurial project without problems.

OK, copyright I was never sure of how that works.

The UI file change was done with a nice utility called designer-qt4 on my system.  Use "designer-qt4 settings-dialog.ui &" at the command line.  It's all pretty self explanatory.  Clicking on an item in Object Inspector, QTabWidget, Signal/Slot brings up properties, etc.  Can select, copy and edit objects on the layout and changes will show up in the windows Object Inspector, etc.  This utility would be nice for more advanced features, but for the smaller things like the errordlg, questdlg, it is just as easy to construct those using a few lines of code.

Dan Sebald <sebald>
Tue 30 Apr 2013 06:54:27 PM UTC, comment #1: 

Why are you using fprintf (stderr, ...)?  Are those debugging statements that you intended to remove?

Can you include the png files in the changeset?

If you create new files, fix the copyright line to have just the current year and your name.

How did you create the addition to the .ui file (that's just a general how-to-use-qt question)?

John W. Eaton <jwe>
Group administrator
Mon 08 Apr 2013 09:36:43 PM UTC, original submission:  

The attached changeset adds a marker class for the purpose of translating Octave core debugger line numbers to line numbers in the edit area which become out of synch if the user edits the file.  I think the behavior is reasonably good so that the edit area doesn't need to be restricted from editing when in debug mode.  The only restriction is that a breakpoint cannot be set in a file that has been edited, and I think that is reasonable.

If the debugger position lands on a breakpoint, the editor line number can be gotten from the marker object.  The marker object retains both the original number (from Octave debugger) and the marker handle--from that it can figure out editor line number.  Otherwise, I call it "unsure position" and use a "gray" arrow to indicate where position is.  There are instances where the breakpoint can be unsure, and I created a grayed redled as well.

This probably needs a slight bit of work yet.  There could be some things going on at the Octave core debugger level not accounted for yet.  See comments below about buggy behavior.


1) I'm fairly certain that no matter what one chooses for a behavior, the marker handle will have to be retained in some way.  Removing a marker via handle is a sure bet.  Removing a marker by saying remove a marker of this type at this debugger line is problematic if the file has been edited.  In other words, the method of this changeset is very robust at keeping track of breakpoints.  The pre-changeset approach was prone to all sorts of markers stuck in the left margin.  (If there are some stuck markers now, it is because the debugger core goes out of synch with the original marker line numbers.)

2) Bookmarks are fine as is, because there is no interaction with the debugger.  The line number is retrieved from the editor area, which naturally agrees with bookmark line number.  No mods for that.

3) QsciScintilla will move markers appropriately, but it doesn't remove a marker when the line it is attached to is deleted.  This is a bit of a problem and in some sense almost a bug.  Delete a line where there is a marker and the marker is now associated with a different line.  Undo the removed lines and the marker stays with the post-delete line, i.e., does not go back to the original marker line.  There is a signal to indicate when the number of lines changes.  I've attached a slot to that and printed out the line number but found it not of much use.  There's no indication that the marker's line has been deleted.  What QsciScintilla should have is a signal "markerLineDeleted (int mhandle)" and "markerLineUndeleted (int mhandle)".  With that, the breakpoint could be greyed to indicate that there is a breakpoint somewhere around there, but unsure exactly where.

There is a "markerFindNext" and "markerFindPrevious" for QsciScintilla, but I'm not sure that is of any help with the translation between debugger line number and editor line number.  I've put some slots in the marker object labelled FUTURE SUPPORT that could be connect to a markerLineDeleted() and markerLineUndelete() signals.  I'm going to post to the QScintilla list and lobby for such signals.

4) Restoring breakpoints upon saving a file is more an editor type of thing.  The file_editor_tab code issues a clear of all breakpoints then attempts to restore them one at a time using the editor line numbers.  Works in theory, but there are issues yet to be resolved.  See #6.

5) There appears to be a bug in the Octave core debugger.  I click on a line inside of a local function and Octave core indicates there is a debug breakpoint there.  (I've printed out messages to stderr inside libinterp/interpfcn/debug.cc to confirm it is Octave core declaring successful add breakpoint.)  However, neither "dbstatus" indicates such a breakpoint, nor does the debugger break at that point when run.  However, I'm able to step into local functions, so I would imagine setting breakpoints there shouldn't be too much extra work.  Also, notice in the local functions that even non-valid command lines can have a breakpoint.  I'll wait until this patch is applied to write a bug report for that.

6) Beyond the bug/behavior in #5, the debugger still seems a bit buggy when it comes to saving a file and trying to restablish breakpoints.  Often it seems like the debugger is getting the lines wrong or there is some kind of conflict.  Furthermore, a little while later after trying to add/delete more breakpoints the alignment kind of corrects itself.  What I'm wondering is whether there might be some synchronization problems.  To illustrate this, try putting some breakpoints in the testfun.m and then add a line or two and save.  It just doesn't seem to get things right when it shouldn't be too difficult.  I'm pretty certain there is no way of losing track of the line number in files with what the changeset setup.  The save sequence I'm using is

clear_all_breakpoints --> callback
save the file
individual add_breakpoint --> callback

What worries me a bit is that saving the file is not in synch with the Octave process.  The clear_all_breakpoints in the Octave core goes through the list and removes individually.  Perhaps it would be better to just delete all breakpoints and notify that all of them have been deleted.  Anyway, I've left some comments in the code so that someone might track what is happening.

7) Upon opening a file, I've tried placing breakpoints if there are some set.  That information is gotten via dbstatus.  However, the callbacks and so on are a bit of a barrier, or I don't understand them well enough.  I've put some signals and slots in the code as placeholders illustrating a more general (and what I think is a preferred) approach.  Queue up an octave command for which the result is eventually signaled back to the object.

8) Sometimes it feels like maybe, at least conceptually, a list of markers would do as well a the signal/slot connections.  I suppose at the center of it all, the signal/slots are a sort of the list management.

9) For the "unsure" marker, I initially didn't like all gray because it didn't contrast enough with the margin.  So I created a custom icon in gimp with a bit of desaturation and copy/paste.  I later tried the QIcon effects of QIcon::Disabled and QIcon::Selected.  The "Selected" isn't too bad.  So, I made this an option under Preferences... "Debugger" menu.  We can pick just one at a later time if people prefer no option.  It's a bit funny in the sense that selecting the icon has the effect of "Selected" for all of them...LOL.

Dan Sebald <sebald>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #28064:  octave-breakpoint-2013may10.patch added by sebald (47KiB - application/octet-stream)
file #28020:  octave-breakpoint-2013may04.patch added by sebald (48KiB - application/octet-stream)
file #27816:  redled_gray.png added by sebald (475B - image/png)
file #27815:  arrow_right_gray.png added by sebald (3KiB - image/png)
file #27814:  octave-breakpoint-2013apr08.patch added by sebald (42KiB - application/octet-stream)
file #27817:  testfun.m added by sebald (133B - application/vnd.wolfram.mathematica.package)

 

Depends on the following items: None found

Items that depend on this one

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by andy1978 (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by ttl (Updated the item)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by sebald (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 logged-in users can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-03-03 lachlan StatusReady For Test Done
        Open/ClosedOpen Closed
    2015-12-29 andy1978 Dependencies- bugs #46779 is dependent
    2015-12-29 ttl StatusNone Ready For Test
        Assigned toNone ttl
    2014-12-31 ttl Attached File- Added octave-breakpoint-2014-12-31.patch, #32739
    2013-05-10 sebald Attached File- Added octave-breakpoint-2013may10.patch, #28064
    2013-05-05 sebald Attached File- Added octave-breakpoint-2013may04.patch, #28020
    2013-04-08 sebald Attached File- Added octave-breakpoint-2013apr08.patch, #27814
        Attached File- Added arrow_right_gray.png, #27815
        Attached File- Added redled_gray.png, #27816
        Attached File- Added testfun.m, #27817

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code