Author Topic: push_tag / pop_bookmark  (Read 9185 times)

jporkka

  • Senior Community Member
  • Posts: 140
  • Hero Points: 6
push_tag / pop_bookmark
« on: March 19, 2008, 08:37:02 PM »
I often use "push_tag" (ctrl-.) to drill down into a call tree to many levels.
This works great.
But many times it seems that pop_bookmark (ctrl-,) pops more than 1 level of tags off the stack.

For example, I'm in F1() and it calls F2(). push_tag on  F2().
Then inside of F2() I see it uses blah->member.
I push_tag on "member" to see where it is defined inside some struct.
Then I pop_bookmark, I find myself back inside of F1() instead of where I was last in F2().

Is this a bug or feature?

Is there some way to see the tag-stack created by push_tag?


This happens with V12 and the beta version on Windows.

ScottW, VP of Dev

  • Senior Community Member
  • Posts: 1471
  • Hero Points: 64
Re: push_tag / pop_bookmark
« Reply #1 on: March 19, 2008, 09:21:03 PM »
Any chance you could provide a code sample for which this is reproducible? I've never seen that behavior. I have to believe it's a weird special case are we'd really be hearing about this.

jporkka

  • Senior Community Member
  • Posts: 140
  • Hero Points: 6
Re: push_tag / pop_bookmark
« Reply #2 on: March 19, 2008, 09:54:12 PM »
Happens to me all the time, in C# and C++.

I do not have a nice neat repro - but I will see what I can do.
It isn't flakey though -- for a given push_tag path it seems to always either work or always not.

MartyL

  • Senior Community Member
  • Posts: 166
  • Hero Points: 29
  • Synergex
Re: push_tag / pop_bookmark
« Reply #3 on: March 19, 2008, 10:08:13 PM »
This is a feature that I use constantly as I work my way through huge call trees. I have never noticed the behavior you're describing. Have you customized alot of the macros? Is there any chance that pop_bookmark is actually getting called twice?

evanratt

  • Senior Community Member
  • Posts: 300
  • Hero Points: 23
Re: push_tag / pop_bookmark
« Reply #4 on: March 21, 2008, 09:32:15 PM »
Sounds like what was reported way back in http://community.slickedit.com/index.php?topic=1517.0 . I can't say I've noticed it recently, so I don't know what I did to stop it from happening. But as I recall, once the bookmark stack got messed up I would pretty much have to restart SlickEdit for it to work correctly again (until the stack got messed up again, at least).

Out of curiosity, what is your maximum bookmark stack depth set to? Mine is cranked up to 100, and I think I turned it up that high when this problem started happening for me (don't know if it's why I don't experience the problem anymore, or totally unrelated).

SlickEdit: Is it possible that there's a bug that becomes apparent when more bookmarks are pushed than the stack can handle?

-Evan

chrisant

  • Senior Community Member
  • Posts: 1410
  • Hero Points: 131
Re: push_tag / pop_bookmark
« Reply #5 on: March 23, 2008, 12:48:46 AM »
This happens all the time to me, as well.  My bookmark stack depth is still the default 15, and I have "Show pushed bookmarks" enabled.  I've noticed that once the bookmark stack gets messed up, I can pop as far back as Ctrl+, will let me without ever visiting the most recently pushed spot that I was trying to go back to.  But if I manually go to the spot I expected to reach, its left edge still shows a pushed bookmark there, as if popping completely skipped it.  That led me to wonder if there might be an off-by-one bug at the max stack depth.  Or perhaps once full the bookmark stack drops the most recently pushed bookmarks, instead of the least recently pushed bookmarks.  I haven't investigated that particular issue yet, but when I get around to it I'll post whatever I'm able to discover.

chrisant

  • Senior Community Member
  • Posts: 1410
  • Hero Points: 131
Re: push_tag / pop_bookmark
« Reply #6 on: March 23, 2008, 07:47:01 PM »
More info:
Bookmark stack depth:  15
Show pushed bookmarks:  ON
Close deletes pushed bookmarks:  ON
def_top_bottom_push_bookmark:  ON

With these settings, I just noticed that when I pressed Ctrl+Home in the Regex Evaluator it pushed a bookmark in the Regex Evaluator (which gets removed when I close the Regex Evaluator).  Probably unrelated, but it made me wonder if there might be somewhere else that is getting pushed bookmarks that shouldn't, and perhaps confuses the stack (or the processing of the stack).


jporkka

  • Senior Community Member
  • Posts: 140
  • Hero Points: 6
Re: push_tag / pop_bookmark
« Reply #7 on: April 01, 2008, 04:58:15 PM »
It does appear to be a bookmark stack overflow.
In V13 there is now a Search->BookmarkStack (bookmark-stack) function which displays the current stack.
I found myself with a confused bookmark stack situation today.
I carefully stepped deep, then out again, and recorded the stack at each step - clearly there are some problems here.

chrisant

  • Senior Community Member
  • Posts: 1410
  • Hero Points: 131
Re: push_tag / pop_bookmark
« Reply #8 on: April 01, 2008, 05:51:53 PM »
Since upgrading to SE 2008 I continue to run into the corrupted stack problem on a daily basis.  Each time I've checked the new Search->Bookmark Stack command, and there is an outlying bookmark #15 that is unreachable.  I'm working through some other stuff at the moment (p_readonly_mode vs _QReadOnly()) but will try to find detailed repro steps for the corrupted bookmark stack hopefully sometime this week.

In the meantime, if you can find repro steps, Joe, that would be helpful.

Dennis

  • Senior Community Member
  • Posts: 3992
  • Hero Points: 520
Re: push_tag / pop_bookmark
« Reply #9 on: April 01, 2008, 06:41:36 PM »
I believe the problem can be attributed the the following setting:

Close deletes pushed bookmarks:  ON

To isolate the problem, please turn this off and see if you still have problems with bookmarks.

chrisant

  • Senior Community Member
  • Posts: 1410
  • Hero Points: 131
Re: push_tag / pop_bookmark
« Reply #10 on: April 01, 2008, 08:52:56 PM »
No, I still get a corrupted bookmark stack with def_cleanup_pushed_bookmarks_on_quit=0.
But now I see the two problems:  (Ok maybe there are more, but these are the ones I see ;)).

Problem #1
The bookmark stack is a circular queue, with _bm_number being the current "next" position in the stack.  _cbquit_bookmarks() removes bookmarks, but does not adjust the circular queue.  I think this is the problem Dennis sees.

Problem #2
As I hypothesized earlier, pop_bookmark() indeed has an off-by-one bug.  When the bookmark stack wraps around, the max bookmark becomes unreachable.  This explains the behavior I've been running into where pop sometimes skips a bookmark that I know is there.
See pushtag.e, circa line 395, SE 2008 revision of the file:
Code: [Select]
_command pop_bookmark() name_info(','VSARG2_ICON|VSARG2_READ_ONLY|VSARG2_REQUIRES_MDI_EDITORCTL)
{
   // check if we are in a push-tag multiple matches ring
   if (maybe_goto_prev_match()) {
      return 0;
   }

   // get the original bookmark number
   i := 0;
   orig_bm_number := _bm_number;
   if (_bm_number <= 1) {
      _bm_number=def_max_bm_tags;
// CHRISANT:  That should be "_bm_number=def_max_bm_tags+1".
// The +1 is needed to counteract the "--_bm_number" that occurs
// as the first statement inside the loop, below.
// Otherwise when the stack wraps around, the max bookmark is
// always orphaned and becomes unreachable.
   }

   // loop until we find a bookmark that is set
   bm_id := "";
   while (_bm_number >= 1) {
      --_bm_number;
      bm_msg := get_message(VSRC_PUSHED_BOOKMARK_NAME);
      if (bm_msg=='') bm_msg="TAG";
      bm_id = bm_msg:+_bm_number;
      i = _BookmarkFind(bm_id,VSBMFLAG_PUSHED);
      if (i >= 0) break;
   }

Dennis

  • Senior Community Member
  • Posts: 3992
  • Hero Points: 520
Re: push_tag / pop_bookmark
« Reply #11 on: April 01, 2008, 10:01:58 PM »
Yes, both issues are real problems.  I had also discovered Problem #2, too late, of course. 

There are larger issues with the Bookmark Stack window because of the nature of the circular queue, the bookmark id's it displays have nothing to do with stack order.  We will be putting together a hot fix for all of this in the near future.

chrisant

  • Senior Community Member
  • Posts: 1410
  • Hero Points: 131
Re: push_tag / pop_bookmark
« Reply #12 on: April 02, 2008, 02:07:42 AM »
I don't know if this will help you with a hotfix, but here is the code I wrote to rotate and shift the bookmark stack to address problem #1.  It's hot off the presses, and may have bugs, but in simple smoke tests it is working for me so far.  It's not as elegant as I'd like, but it's a starting point, at least.

Code: [Select]
/**
 * Ideally this would be a built in function that simply renames the bookmark.
 * Until such a function is available, this attempts to achieve the effect of
 * renaming a bookmark by getting its information, deleting it, and creating a
 * new bookmark.
 *
 * @param old_name   Name of existing pushed bookmark.
 * @param new_name   New name for the pushed bookmark.
 *
 * @return int       0 on success, or non-zero on failure.
 */
static int RenamePushedBookmark(_str old_name, _str new_name)
{
   if (old_name :== new_name) {
      return 0;
   }

   // bookmark attributes
   bm_name  := '';
   bm_mark  := 0;
   bm_flags := 0;
   bm_bufid := 0;
   bm_line  := 0;
   bm_rline := 0;
   bm_col   := 0;
   bm_rpos  := 0;
   bm_data  := '';
   bm_file  := '';
   bm_docname := '';

   // get old bookmark
   int k = _BookmarkFind(old_name, VSBMFLAG_PUSHED);
   if (k < 0) {
      return -1;
   }
   _BookmarkGetInfo(k, bm_name, bm_mark, bm_flags, bm_bufid,
                       bm_line, bm_rline, bm_col, bm_rpos, bm_data,
                       bm_file, bm_docname);
   if (bm_bufid < 0) {
      return -1;
   }

   // remove the existing bookmark, but pass 0 to avoid freeing bm_mark
   _BookmarkRemove(k, 0);

   // create a new bookmark
   int temp_view_id;
   int orig_view_id;
   int status = _open_temp_view('', temp_view_id, orig_view_id, '+bi 'bm_bufid);
   _BookmarkAdd(new_name, bm_mark, bm_flags, bm_rline, bm_col, bm_rpos, bm_data, bm_file, bm_docname);
   _delete_temp_view(temp_view_id);
   activate_window(orig_view_id);
   return 0;
}

/**
 * Gets called when a buffer is closed.
 * This function is used to clean up pushed bookmarks from
 * the bookmark stack when a file is closed by the user.
 *
 * @param buffid  p_buf_id of the buffer that was closed
 * @param name    p_buf_name of the buffer that was closed
 * @param docname p_DocumentName of the buffer that was closed
 * @param flags   assumed to be 0
 */
void _cbquit_bookmarks(int buffid, _str name, _str docname='', int flags=0)
{
   // is this option turned off?
   if (!def_cleanup_pushed_bookmarks_on_quit) {
      return;
   }

   // bookmark attributes
   bm_name  := '';
   bm_mark  := 0;
   bm_flags := 0;
   bm_bufid := 0;
   bm_line  := 0;
   bm_rline := 0;
   bm_col   := 0;
   bm_rpos  := 0;
   bm_data  := '';
   bm_file  := '';
   bm_docname := '';

   bm_msg := get_message(VSRC_PUSHED_BOOKMARK_NAME);
   if (bm_msg=='') bm_msg="TAG";
   int arrayBookmarkNums[];
   int highestBookmarkNum = 0;
   boolean isCondenseNeeded=false;

   // check each bookmark if it matches the buffer being quit
   n := _BookmarkQCount();
   for (i:=n-1; i>=0; --i) {
      _BookmarkGetInfo(i, bm_name, bm_mark, bm_flags, bm_bufid,
                          bm_line, bm_rline, bm_col, bm_rpos, bm_data,
                          bm_file, bm_docname);
      if (!(bm_flags & VSBMFLAG_PUSHED)) {
         continue;
      }
      boolean isPushedName = (pos(bm_msg, bm_name) == 1 && pos('^\:i$', substr(bm_name, length(bm_msg) + 1), 1, 'U'));
      if (bm_bufid == buffid) {
         _BookmarkRemove(i);
         if (isPushedName) {
            isCondenseNeeded = true;
         }
      } else if (isPushedName) {
         // make an array out of the bookmark stack, so we can condense it
         int bmNum = (int)substr(bm_name, length(bm_msg) + pos('S'));
         arrayBookmarkNums[arrayBookmarkNums._length()]=bmNum;
         if (highestBookmarkNum < bmNum) {
            highestBookmarkNum = bmNum;
         }
      }
   }

   // Condense the bookmark stack if any bookmarks were removed from it.  The
   // circular stack will be rotated and shifted so that the first bookmark is
   // named 1, and _bm_number-1 (before wrap around) is the highest bookmark.
   if (isCondenseNeeded) {
      // First move any bookmarks lower than _bm_number to be greater.  This
      // effectively rotates the circular stack, but does not yet shift it.
      int ii;
      arrayBookmarkNums._sort('N');
      for (ii = 0; ii < arrayBookmarkNums._length(); ii++) {
         if (arrayBookmarkNums[ii] < _bm_number) {
            RenamePushedBookmark(bm_msg :+ arrayBookmarkNums[ii],
                                 bm_msg :+ (arrayBookmarkNums[ii] + highestBookmarkNum + 1));
         }
      }
      // Defer rotating arrayBookmarkNums until the stack has been rotated.
      // Otherwise it is hard to keep track of what remains to be rotated.
      for (ii = arrayBookmarkNums._length(); ii--;) {
         if (arrayBookmarkNums[ii] < _bm_number) {
            arrayBookmarkNums[arrayBookmarkNums._length()] = arrayBookmarkNums[ii] + highestBookmarkNum + 1;
            arrayBookmarkNums._deleteel(ii);
         }
      }
      // Then shift the bookmark stack to start at 1, and set _bm_number to
      // the number after the highest bookmark (wrapping around to 1 if
      // necessary).
      arrayBookmarkNums._sort('N');
      for (ii = 0; ii < arrayBookmarkNums._length(); ii++) {
         RenamePushedBookmark(bm_msg :+ arrayBookmarkNums[ii],
                              bm_msg :+ (ii + 1));
      }
      _bm_number = arrayBookmarkNums._length() + 1;
      if (_bm_number > def_max_bm_tags) {
         _bm_number = 1;
      }
   }
}

EDIT:  Oops, had neglected to wrap _bm_number if there were exactly def_max_bm_tags pushed bookmarks.
« Last Edit: April 02, 2008, 02:31:41 AM by chrisant »

Dennis

  • Senior Community Member
  • Posts: 3992
  • Hero Points: 520
Re: push_tag / pop_bookmark
« Reply #13 on: April 02, 2008, 01:46:35 PM »
@chrisant:  Thanks for your efforts.  In fact, when you get the hot fix you will find that we just got rid of _bm_number and the circular queue and now have a real stack, which works much, much better and is less code.