Re: bookmark.el bug report

[Karl Fogel]

"Drew Adams" <address@hidden> writes:
>Do as you like, but part of my message was that there is no need to call
>`bookmark-bmenu-check-position' here (which you renamed `...ensure...').
>
>Since the "check" part of that function doesn't really do anything, all
>it really does is move point to the nearest bookmark line. If point is
>already on a bookmark line, then it is a no-op.

Sure.  But when point is *not* on a bookmark line (for example, if it is
at the beginning or end of the buffer), then it should be moved to a
bookmark line.  IMHO, the mode works much more intuitively if it behaves
that way.

>And the way to tell if point is on a bookmark line is (now) to call
>`bookmark-bmenu-bookmark'. So I use this simplified code (FWIW):
>
>(defun bookmark-bmenu-check-position ()
>  "Move to the beginning of the nearest bookmark line."
>  (beginning-of-line)
>  (unless (bookmark-bmenu-bookmark)
>    (if (and (bolp) (eobp))
>        (beginning-of-line 0)
>      (goto-char (point-min))
>      (forward-line bookmarkp-bmenu-header-lines)))
>  t) ; Vanilla bookmark code depends on non-nil value.
>
>(defun bookmark-bmenu-bookmark ()
>  "Return the name of the bookmark on this line."
>  (condition-case nil
>      (save-excursion
>       (forward-line 0)
>       (forward-char (1+ bookmarkp-bmenu-marks-width))
>       (get-text-property (point) 'bookmarkp-bookmark-name))
>    (error nil)))
>
>Instead of calling `*-bmenu-bookmark' at the beginning of `ensure' to check for
>a no-op, the original code has it backward: it calls `ensure' at the start of
>each call to `*-bmenu-bookmark', to ensure that point is on a bookmark
>line. But for many calls to `*-bmenu-bookmark', we already know that
>point is on a bookmark line.
>
>If it is not called also within `*-bmenu-bookmark', then `ensure' is
>called much less often than `*-bmenu-bookmark'. And the original code
>has extra calls to `ensure', besides the one in `*-bmenu-mark'. In
>fact, it often calls it both at the beginning and at the end of a
>function (e.g. `*-bmenu-delete').
>
>`ensure' only needs to be called before some (not all) of the calls to
>`*-bmenu-bookmark' - it has no use otherwise.
>
>That's why I said you can get rid of some of the calls to `ensure'. But
>if `ensure' is not called from within `*-bmenu-bookmark' then you need
>to protect the eob case (just to inhibit a msg) - hence the
>`condition-case'.

I believe that most of the calls to `-ensure-' can now be gotten rid of
(though probably not all, e.g., in `bookmark-bmenu-mark').  However, I
like to move one step at a time -- and the first step was to get rid of
the tests, not the calls themselves.  I just need to take a separate
look regarding the calls.

>I don't know what changes you actually made, since I cannot see them here:
>http://cvs.savannah.gnu.org/viewvc/emacs/emacs/lisp/
>The last revision of bookmark.el shown there dates from Nov 24.

Yeah, CVS has been read-only since we switched to Bazaar some days ago,
and will not be receiving any more changes.

>Has the URL for the Emacs development source code changed, now that the
>repository is on BZR? If so, what is it - how does one get to the
>source code via HTTP? And if that URL is no longer valid, then its Web
>page should redirect to the proper URL, no?

Yes, it has changed.  http://www.emacswiki.org/emacs-en/BzrForEmacsDevs
has the details.

>Personally, I do not intend to access the repository using BZR (just as
>I did not use CVS to access it before). I'm looking for a way to
>download a revision by just clicking an ordinary web-page link in my
>browser, as before.

The URL to start at is http://bzr.savannah.gnu.org/lh/emacs/, but
unfortunately it's broken right now.  There is a ticket filed about
fixing it: https://savannah.gnu.org/support/index.php?107142

Fortunately, the version at Launchpad is working just fine:

  https://code.launchpad.net/~vcs-imports/emacs/trunk

And bookmark.el is here:

http://bazaar.launchpad.net/~vcs-imports/emacs/trunk/annotate/head%3A/lisp/bookmark.el

That should always be up-to-date, give or take a few hours.

-Karl