* bookmark.el bug report
@ 2009-12-28 6:19 Sun Yijiang
2009-12-28 6:48 ` Karl Fogel
2010-01-02 5:05 ` Karl Fogel
0 siblings, 2 replies; 21+ messages in thread
From: Sun Yijiang @ 2009-12-28 6:19 UTC (permalink / raw)
To: emacs-devel
`bookmark-bmenu-execute-deletions' is broken for quite some time. I
think I've found the problem. When it calls
`bookmark-bmenu-bookmark', the callee gets text property at
`(line-begging-position)', which is incorrect in this context. Below
is the patched code of `bookmark-bmenu-bookmark' that works for me:
(defun bookmark-bmenu-bookmark ()
"Return the bookmark for this line in an interactive bookmark list buffer."
(when (bookmark-bmenu-check-position)
(let ((pos (line-beginning-position)))
(when (looking-back "^[^ ]")
(setq pos (+ 1 pos)))
(get-text-property pos 'bookmark-name-prop))))
Sincerely,
Steve
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-28 6:19 bookmark.el bug report Sun Yijiang
@ 2009-12-28 6:48 ` Karl Fogel
2009-12-28 7:41 ` Thierry Volpiatto
2010-01-02 5:05 ` Karl Fogel
1 sibling, 1 reply; 21+ messages in thread
From: Karl Fogel @ 2009-12-28 6:48 UTC (permalink / raw)
To: Sun Yijiang; +Cc: emacs-devel
Sun Yijiang <sunyijiang@gmail.com> writes:
> `bookmark-bmenu-execute-deletions' is broken for quite some time. I
> think I've found the problem. When it calls
> `bookmark-bmenu-bookmark', the callee gets text property at
> `(line-begging-position)', which is incorrect in this context. Below
> is the patched code of `bookmark-bmenu-bookmark' that works for me:
>
> (defun bookmark-bmenu-bookmark ()
> "Return the bookmark for this line in an interactive bookmark list buffer."
> (when (bookmark-bmenu-check-position)
> (let ((pos (line-beginning-position)))
> (when (looking-back "^[^ ]")
> (setq pos (+ 1 pos)))
> (get-text-property pos 'bookmark-name-prop))))
Thank you -- I will take a look!
-Karl
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-28 6:48 ` Karl Fogel
@ 2009-12-28 7:41 ` Thierry Volpiatto
2009-12-28 15:58 ` Drew Adams
0 siblings, 1 reply; 21+ messages in thread
From: Thierry Volpiatto @ 2009-12-28 7:41 UTC (permalink / raw)
To: emacs-devel
Karl Fogel <kfogel@red-bean.com> writes:
> Sun Yijiang <sunyijiang@gmail.com> writes:
>> `bookmark-bmenu-execute-deletions' is broken for quite some time. I
>> think I've found the problem. When it calls
>> `bookmark-bmenu-bookmark', the callee gets text property at
>> `(line-begging-position)', which is incorrect in this context. Below
>> is the patched code of `bookmark-bmenu-bookmark' that works for me:
>>
>> (defun bookmark-bmenu-bookmark ()
>> "Return the bookmark for this line in an interactive bookmark list buffer."
>> (when (bookmark-bmenu-check-position)
>> (let ((pos (line-beginning-position)))
>> (when (looking-back "^[^ ]")
>> (setq pos (+ 1 pos)))
>> (get-text-property pos 'bookmark-name-prop))))
>
> Thank you -- I will take a look!
Consider using the definition of bookmark-extensions.el:
(That's what i sent in original patch for bookmark.el)
(defun bookmark-bmenu-bookmark ()
"Return the name of the bookmark on this line."
(when (bookmark-bmenu-check-position)
(save-excursion
(forward-line 0) (forward-char 3)
(get-text-property (point) 'bmkext-bookmark-name))))
--
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: bookmark.el bug report
2009-12-28 7:41 ` Thierry Volpiatto
@ 2009-12-28 15:58 ` Drew Adams
2009-12-28 21:12 ` Karl Fogel
0 siblings, 1 reply; 21+ messages in thread
From: Drew Adams @ 2009-12-28 15:58 UTC (permalink / raw)
To: emacs-devel
> (defun bookmark-bmenu-bookmark ()
> "Return the name of the bookmark on this line."
> (when (bookmark-bmenu-check-position)
> (save-excursion
> (forward-line 0) (forward-char 3)
> (get-text-property (point) 'bmkext-bookmark-name))))
1. There is no need for the `bookmark-bmenu-check-position' test. All such
pseudotests can be removed - that function always returns non-nil (unless the
`bookmark-alist' is nil).
2. Wrap the `save-excursion' in `ignore-errors' or a `condition-case', for the
`forward-char' at eob. E.g., this is the definition in bookmark+.el:
(defun bookmark-bmenu-bookmark ()
"Return the name of the bookmark on this line."
(condition-case nil
(save-excursion
(forward-line 0) (forward-char 3)
(get-text-property (point) 'bookmarkp-bookmark-name))
(error nil)))
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-28 15:58 ` Drew Adams
@ 2009-12-28 21:12 ` Karl Fogel
2009-12-29 18:22 ` Stefan Monnier
0 siblings, 1 reply; 21+ messages in thread
From: Karl Fogel @ 2009-12-28 21:12 UTC (permalink / raw)
To: Drew Adams; +Cc: emacs-devel
"Drew Adams" <drew.adams@oracle.com> writes:
>> (defun bookmark-bmenu-bookmark ()
>> "Return the name of the bookmark on this line."
>> (when (bookmark-bmenu-check-position)
>> (save-excursion
>> (forward-line 0) (forward-char 3)
>> (get-text-property (point) 'bmkext-bookmark-name))))
>
> 1. There is no need for the `bookmark-bmenu-check-position' test. All such
> pseudotests can be removed - that function always returns non-nil (unless the
> `bookmark-alist' is nil).
I have a pending change for that (in my CVS working copy -- need to port
it over to Bzr). I want to make absolutely sure there wasn't some edge
case about when bookmark-alist is nil, but I strongly suspect you're
right, Drew.
> 2. Wrap the `save-excursion' in `ignore-errors' or a `condition-case',
> for the `forward-char' at eob. E.g., this is the definition in
> bookmark+.el:
>
> (defun bookmark-bmenu-bookmark ()
> "Return the name of the bookmark on this line."
> (condition-case nil
> (save-excursion
> (forward-line 0) (forward-char 3)
> (get-text-property (point) 'bookmarkp-bookmark-name))
> (error nil)))
Will take into account.
Thanks,
-K
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-28 21:12 ` Karl Fogel
@ 2009-12-29 18:22 ` Stefan Monnier
2009-12-30 13:37 ` Thierry Volpiatto
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Monnier @ 2009-12-29 18:22 UTC (permalink / raw)
To: Karl Fogel; +Cc: Drew Adams, emacs-devel
>> (condition-case nil
>> (save-excursion
>> (forward-line 0) (forward-char 3)
Hardcoding 3 is ugly (I know it's done elsewhere, but it's still ugly,
elsewhere a well).
>> (get-text-property (point) 'bookmarkp-bookmark-name))
>> (error nil)))
> Will take into account.
Better would be to place the text property at BOL so there's no need for
any forward-char business.
Stefan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-29 18:22 ` Stefan Monnier
@ 2009-12-30 13:37 ` Thierry Volpiatto
2009-12-30 15:28 ` Stefan Monnier
0 siblings, 1 reply; 21+ messages in thread
From: Thierry Volpiatto @ 2009-12-30 13:37 UTC (permalink / raw)
To: emacs-devel
Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>>> (condition-case nil
>>> (save-excursion
>>> (forward-line 0) (forward-char 3)
>
> Hardcoding 3 is ugly (I know it's done elsewhere, but it's still ugly,
> elsewhere a well).
That can be used instead:
(skip-chars-forward " \\|\*\\|>\\|D")
>>> (get-text-property (point) 'bookmarkp-bookmark-name))
>>> (error nil)))
>
>> Will take into account.
>
> Better would be to place the text property at BOL so there's no need for
> any forward-char business.
Yes, but remember you have to set properties on the bookmark name
without "*"(annotation), ">"(mark), "D"(mark for deletion).
That occur on third char and not before.
--
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-30 13:37 ` Thierry Volpiatto
@ 2009-12-30 15:28 ` Stefan Monnier
2009-12-30 15:57 ` Drew Adams
2009-12-30 17:43 ` Thierry Volpiatto
0 siblings, 2 replies; 21+ messages in thread
From: Stefan Monnier @ 2009-12-30 15:28 UTC (permalink / raw)
To: Thierry Volpiatto; +Cc: emacs-devel
>> Better would be to place the text property at BOL so there's no need for
>> any forward-char business.
> Yes, but remember you have to set properties on the bookmark name
> without "*"(annotation), ">"(mark), "D"(mark for deletion).
> That occur on third char and not before.
Ah, that makes sense.
So maybe hardcoding 2 is not such a bad idea, but the important thing
is then that the same "hardcoding" be used both where the property is used
and where it's set.
Stefan
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: bookmark.el bug report
2009-12-30 15:28 ` Stefan Monnier
@ 2009-12-30 15:57 ` Drew Adams
2009-12-30 16:26 ` Drew Adams
2009-12-30 17:43 ` Thierry Volpiatto
1 sibling, 1 reply; 21+ messages in thread
From: Drew Adams @ 2009-12-30 15:57 UTC (permalink / raw)
To: emacs-devel
> > >>> (forward-char 3)
> > >
> > > Hardcoding 3 is ugly (I know it's done elsewhere, but it's
> > > still ugly, elsewhere a well).
> >
> > That can be used instead:(skip-chars-forward " \\|\*\\|>\\|D")
We are essentially moving to a different "field" here (even if not an Emacs
field per se). The field at bol is a marks field; the field starting at char 3
is the bookmark-name field.
IMO, it is worse to base the movement on syntax, skipping over the marks (and
lack of marks) that are currently used in the design. That complicates changing
the set of marks in the future, and it precludes bookmarks with names that start
with any such marks (including SPC).
Even if we never want either of those possibilities, such a search is more
fragile, and it couples the two fields closely wrt code. The marks field should
not be defined by its syntax (vs a bookmark-name syntax) but by its columns. It
is a field of a fixed number of mark columns, regardless of which marks are
used.
It is better to skip over the width of the field.
Either (1) comment the hard-coded field width (everywhere it is hard-coded, as
Stefan suggested) or, better, (2) add a variable for that width.
> >> Better would be to place the text property at BOL so
> >> there's no need for any forward-char business.
> >
> > Yes, but remember you have to set properties on the bookmark name
> > without "*"(annotation), ">"(mark), "D"(mark for deletion).
> > That occur on third char and not before.
>
> Ah, that makes sense.
> So maybe hardcoding 2 is not such a bad idea, but the important thing
> is then that the same "hardcoding" be used both where the
> property is used and where it's set.
Yes. And to enforce that, either add comments to that effect or, preferably, use
a variable.
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: bookmark.el bug report
2009-12-30 15:57 ` Drew Adams
@ 2009-12-30 16:26 ` Drew Adams
0 siblings, 0 replies; 21+ messages in thread
From: Drew Adams @ 2009-12-30 16:26 UTC (permalink / raw)
To: emacs-devel
> It is better to skip over the width of the field.
> Either (1) comment the hard-coded field width (everywhere it
> is hard-coded, as Stefan suggested) or, better, (2) add a
> variable for that width.
Another candidate for this is the number of header lines (also 2). Something
like this:
(defconst bookmark-bmenu-header-lines 2
"Number of lines used for the *Bookmark List* header.")
(defconst bookmark-bmenu-marks-width 2
"Number of columns (chars) used for the *Bookmark List* marks column.")
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-30 15:28 ` Stefan Monnier
2009-12-30 15:57 ` Drew Adams
@ 2009-12-30 17:43 ` Thierry Volpiatto
2009-12-30 18:32 ` Stefan Monnier
1 sibling, 1 reply; 21+ messages in thread
From: Thierry Volpiatto @ 2009-12-30 17:43 UTC (permalink / raw)
To: emacs-devel
Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>>> Better would be to place the text property at BOL so there's no need for
>>> any forward-char business.
>> Yes, but remember you have to set properties on the bookmark name
>> without "*"(annotation), ">"(mark), "D"(mark for deletion).
>> That occur on third char and not before.
>
> Ah, that makes sense.
> So maybe hardcoding 2 is not such a bad idea, but the important thing
> is then that the same "hardcoding" be used both where the property is used
> and where it's set.
Yes, it's why start was set to (point + 2) in *-bmenu-list in my original
patch.(It is like that in bookmark-extensions.el).
--
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-30 17:43 ` Thierry Volpiatto
@ 2009-12-30 18:32 ` Stefan Monnier
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Monnier @ 2009-12-30 18:32 UTC (permalink / raw)
To: Thierry Volpiatto; +Cc: emacs-devel
>>>> Better would be to place the text property at BOL so there's no need for
>>>> any forward-char business.
>>> Yes, but remember you have to set properties on the bookmark name
>>> without "*"(annotation), ">"(mark), "D"(mark for deletion).
>>> That occur on third char and not before.
>> Ah, that makes sense.
>> So maybe hardcoding 2 is not such a bad idea, but the important thing
>> is then that the same "hardcoding" be used both where the property is used
>> and where it's set.
> Yes, it's why start was set to (point + 2) in *-bmenu-list in my original
> patch.(It is like that in bookmark-extensions.el).
Yes, that's what I figured in the mean time.
So we should probably revert to the "+2" (tho it also has its
downside). But please use a defconst to hold the "2".
Stefan
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2009-12-28 6:19 bookmark.el bug report Sun Yijiang
2009-12-28 6:48 ` Karl Fogel
@ 2010-01-02 5:05 ` Karl Fogel
2010-01-02 5:15 ` Karl Fogel
2010-01-02 7:51 ` Drew Adams
1 sibling, 2 replies; 21+ messages in thread
From: Karl Fogel @ 2010-01-02 5:05 UTC (permalink / raw)
To: Sun Yijiang; +Cc: emacs-devel
Sun Yijiang <sunyijiang@gmail.com> writes:
>`bookmark-bmenu-execute-deletions' is broken for quite some time. I
>think I've found the problem. When it calls
>`bookmark-bmenu-bookmark', the callee gets text property at
>`(line-begging-position)', which is incorrect in this context. Below
>is the patched code of `bookmark-bmenu-bookmark' that works for me:
>
>(defun bookmark-bmenu-bookmark ()
> "Return the bookmark for this line in an interactive bookmark list buffer."
> (when (bookmark-bmenu-check-position)
> (let ((pos (line-beginning-position)))
> (when (looking-back "^[^ ]")
> (setq pos (+ 1 pos)))
> (get-text-property pos 'bookmark-name-prop))))
Okay, this is fixed in the two revisions named below. Thank you for the
bug report, Yijiang.
Note to Drew Adams: I took your suggestions about new constants.
However, I did not wrap the new `save-excursion' in `ignore-errors' or
`condition-case', because we should never be at eob right after calling
`bookmark-bmenu-ensure-position' (and if we are, I want to know).
-Karl
------------------------------------------------------------
revno: 99230
revision-id: kfogel@red-bean.com-20100102050055-3fnwh3oam9gwoc1i
parent: kfogel@red-bean.com-20100102043617-3bbh6lg62a8ycsa2
committer: Karl Fogel <kfogel@red-bean.com>
branch nick: trunk
timestamp: Sat 2010-01-02 00:00:55 -0500
message:
* lisp/bookmark.el (bookmark-bmenu-marks-width): Define to 1, not 2.
(bookmark-bmenu-list, bookmark-bmenu-bookmark): Calculate property
positions by using `bookmark-bmenu-marks-width', instead of hardcoding.
This fixes the `bookmark-bmenu-execute-deletions' bug reported here:
http://lists.gnu.org/archive/html/emacs-devel/2009-12/msg00819.html
From: Sun Yijiang <sunyijiang {_AT_} gmail.com>
To: emacs-devel {_AT_} gnu.org
Subject: bookmark.el bug report
Date: Mon, 28 Dec 2009 14:19:16 +0800
Message-ID: 5065e2900912272219y3734fc9fsdaee41167ef99ad7@mail.gmail.com
------------------------------------------------------------
revno: 99229
revision-id: kfogel@red-bean.com-20100102043617-3bbh6lg62a8ycsa2
parent: lekktu@gmail.com-20100102010911-q2hnd1wl2vqgfgb0
committer: Karl Fogel <kfogel@red-bean.com>
branch nick: trunk
timestamp: Fri 2010-01-01 23:36:17 -0500
message:
* lisp/bookmark.el: Improvements suggested by Drew Adams:
(bookmark-bmenu-ensure-position): New name for
`bookmark-bmenu-check-position'. Just ensure the position;
don't return any meaningful value.
(bookmark-bmenu-header-height, bookmark-bmenu-marks-width): New constants.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2010-01-02 5:05 ` Karl Fogel
@ 2010-01-02 5:15 ` Karl Fogel
2010-01-02 7:03 ` Thierry Volpiatto
2010-01-02 7:51 ` Drew Adams
1 sibling, 1 reply; 21+ messages in thread
From: Karl Fogel @ 2010-01-02 5:15 UTC (permalink / raw)
To: Sun Yijiang; +Cc: emacs-devel
Karl Fogel <kfogel@red-bean.com> writes:
>------------------------------------------------------------
>revno: 99229
>revision-id: kfogel@red-bean.com-20100102043617-3bbh6lg62a8ycsa2
>parent: lekktu@gmail.com-20100102010911-q2hnd1wl2vqgfgb0
>committer: Karl Fogel <kfogel@red-bean.com>
>branch nick: trunk
>timestamp: Fri 2010-01-01 23:36:17 -0500
>message:
> * lisp/bookmark.el: Improvements suggested by Drew Adams:
> (bookmark-bmenu-ensure-position): New name for
> `bookmark-bmenu-check-position'. Just ensure the position;
> don't return any meaningful value.
> (bookmark-bmenu-header-height, bookmark-bmenu-marks-width): New constants.
And if I could edit the commit message for this one, I'd add:
(everywhere): Adjust former callers of `bookmark-bmenu-check-position'
to call `bookmark-bmenu-ensure-position' instead, and to not use the
return value.
Sigh. I don't miss CVS, but being able to fix log messages is nice.
-K
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2010-01-02 5:15 ` Karl Fogel
@ 2010-01-02 7:03 ` Thierry Volpiatto
2010-01-02 7:26 ` Karl Fogel
0 siblings, 1 reply; 21+ messages in thread
From: Thierry Volpiatto @ 2010-01-02 7:03 UTC (permalink / raw)
To: emacs-devel
Karl Fogel <kfogel@red-bean.com> writes:
> Karl Fogel <kfogel@red-bean.com> writes:
>>------------------------------------------------------------
>>revno: 99229
>>revision-id: kfogel@red-bean.com-20100102043617-3bbh6lg62a8ycsa2
>>parent: lekktu@gmail.com-20100102010911-q2hnd1wl2vqgfgb0
>>committer: Karl Fogel <kfogel@red-bean.com>
>>branch nick: trunk
>>timestamp: Fri 2010-01-01 23:36:17 -0500
>>message:
>> * lisp/bookmark.el: Improvements suggested by Drew Adams:
>> (bookmark-bmenu-ensure-position): New name for
>> `bookmark-bmenu-check-position'. Just ensure the position;
>> don't return any meaningful value.
>> (bookmark-bmenu-header-height, bookmark-bmenu-marks-width): New constants.
>
> And if I could edit the commit message for this one, I'd add:
>
> (everywhere): Adjust former callers of `bookmark-bmenu-check-position'
> to call `bookmark-bmenu-ensure-position' instead, and to not use the
> return value.
There is one bug also in bookmark-bmenu-hide-filenames where you forget
to put properties on bookmark when rebuilding.
So after one use of hide-/show-filenames, bookmark-bmenu-bookmark
doesn't work anymore because bookmark have no properties.
I sent a patch to Stefan.
--
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2010-01-02 7:03 ` Thierry Volpiatto
@ 2010-01-02 7:26 ` Karl Fogel
2010-01-02 8:15 ` Thierry Volpiatto
0 siblings, 1 reply; 21+ messages in thread
From: Karl Fogel @ 2010-01-02 7:26 UTC (permalink / raw)
To: Thierry Volpiatto; +Cc: emacs-devel
Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>There is one bug also in bookmark-bmenu-hide-filenames where you forget
>to put properties on bookmark when rebuilding.
>So after one use of hide-/show-filenames, bookmark-bmenu-bookmark
>doesn't work anymore because bookmark have no properties.
>
>I sent a patch to Stefan.
I can't reproduce this. Can you please try with the latest bookmark.el
and let me know an exact reproduction recipe, if you can reproduce?
Thanks,
-Karl
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: bookmark.el bug report
2010-01-02 5:05 ` Karl Fogel
2010-01-02 5:15 ` Karl Fogel
@ 2010-01-02 7:51 ` Drew Adams
2010-01-02 8:16 ` Karl Fogel
1 sibling, 1 reply; 21+ messages in thread
From: Drew Adams @ 2010-01-02 7:51 UTC (permalink / raw)
To: 'Karl Fogel', 'Sun Yijiang'; +Cc: emacs-devel
> Note to Drew Adams: I took your suggestions about new constants.
> However, I did not wrap the new `save-excursion' in `ignore-errors'
> or `condition-case', because we should never be at eob right
> after calling `bookmark-bmenu-ensure-position' (and if we are,
> I want to know).
Hi Karl,
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.
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'.
HTH.
--
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.
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?
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2010-01-02 7:26 ` Karl Fogel
@ 2010-01-02 8:15 ` Thierry Volpiatto
2010-01-02 19:17 ` Karl Fogel
0 siblings, 1 reply; 21+ messages in thread
From: Thierry Volpiatto @ 2010-01-02 8:15 UTC (permalink / raw)
To: emacs-devel
Karl Fogel <kfogel@red-bean.com> writes:
> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>There is one bug also in bookmark-bmenu-hide-filenames where you forget
>>to put properties on bookmark when rebuilding.
>>So after one use of hide-/show-filenames, bookmark-bmenu-bookmark
>>doesn't work anymore because bookmark have no properties.
>>
>>I sent a patch to Stefan.
>
> I can't reproduce this. Can you please try with the latest bookmark.el
> and let me know an exact reproduction recipe, if you can reproduce?
It is working, but still fragile if you don't put properties on bookmark
when rebuilding in *-hide-filenames.
,----[ bookmark-bmenu-hide-filenames ]
| (while bookmark-bmenu-hidden-bookmarks
| (move-to-column column t)
| (bookmark-kill-line)
| (let ((name (pop bookmark-bmenu-hidden-bookmarks))
| (start (point)))
| (insert name)
| (put-text-property start (point) 'bookmark-name-prop name)
| (if (and (display-color-p) (display-mouse-p))
| (add-text-properties
| start (point)
`----
Also, in the same function,now you have a new variable, there is no need
to set column around bookmark title, just set it to this new variable.
,----[ bookmark-bmenu-hide-filenames ]
| (let ((inhibit-read-only t)
| (column bookmark-bmenu-marks-width))
`----
Why `bookmark-bmenu-marks-width' is set to 1 and not 2?
--
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2010-01-02 7:51 ` Drew Adams
@ 2010-01-02 8:16 ` Karl Fogel
2010-01-02 8:42 ` Drew Adams
0 siblings, 1 reply; 21+ messages in thread
From: Karl Fogel @ 2010-01-02 8:16 UTC (permalink / raw)
To: Drew Adams; +Cc: 'Sun Yijiang', emacs-devel
"Drew Adams" <drew.adams@oracle.com> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: bookmark.el bug report
2010-01-02 8:16 ` Karl Fogel
@ 2010-01-02 8:42 ` Drew Adams
0 siblings, 0 replies; 21+ messages in thread
From: Drew Adams @ 2010-01-02 8:42 UTC (permalink / raw)
To: 'Karl Fogel'; +Cc: 'Sun Yijiang', emacs-devel
> Fortunately, the version at Launchpad is working just fine:
> https://code.launchpad.net/~vcs-imports/emacs/trunk
Shouldn't the obsolete CVS page be automatically redirected here?
http://bazaar.launchpad.net/%7Evcs-imports/emacs/trunk/files/head%3A/lisp/
That seems to be more or less the equivalent of the old page:
http://cvs.savannah.gnu.org/viewvc/emacs/emacs/lisp/
Redirection means bookmarks will continue to work etc.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: bookmark.el bug report
2010-01-02 8:15 ` Thierry Volpiatto
@ 2010-01-02 19:17 ` Karl Fogel
0 siblings, 0 replies; 21+ messages in thread
From: Karl Fogel @ 2010-01-02 19:17 UTC (permalink / raw)
To: Thierry Volpiatto; +Cc: emacs-devel
Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>> I can't reproduce this. Can you please try with the latest bookmark.el
>> and let me know an exact reproduction recipe, if you can reproduce?
>
>It is working, but still fragile if you don't put properties on bookmark
>when rebuilding in *-hide-filenames.
I've fixed it to work the way you recommend, which is more robust than
what I was doing before. Thanks (there was a boundary error in my head
that led me to believe the old way was robust too, but then I remembered
about annotation marks appearing in the second column).
>,----[ bookmark-bmenu-hide-filenames ]
>| (while bookmark-bmenu-hidden-bookmarks
>| (move-to-column column t)
>| (bookmark-kill-line)
>| (let ((name (pop bookmark-bmenu-hidden-bookmarks))
>| (start (point)))
>| (insert name)
>| (put-text-property start (point) 'bookmark-name-prop name)
>| (if (and (display-color-p) (display-mouse-p))
>| (add-text-properties
>| start (point)
>`----
>
>Also, in the same function,now you have a new variable, there is no need
>to set column around bookmark title, just set it to this new variable.
I've gotten rid of the column variable -- you're right, it's no longer
necessary -- and am adding the text property now.
I think you might have an older version of bookmark.el; are you working
with the latest one from the Bazaar master branch? If not, please do.
If you can't get it the usual way, then see [1] for another way. Also,
it would be great if you can send suggested changes in 'patch' form
(i.e., using 'diff -u'). But I can read it the above way too.
>,----[ bookmark-bmenu-hide-filenames ]
>| (let ((inhibit-read-only t)
>| (column bookmark-bmenu-marks-width))
>`----
>
>Why `bookmark-bmenu-marks-width' is set to 1 and not 2?
See above about boundary error. Fixed now :-).
Thanks!
-Karl
[1] http://bazaar.launchpad.net/~vcs-imports/emacs/trunk/annotate/head%3A/lisp/bookmark.el
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2010-01-02 19:17 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-12-28 6:19 bookmark.el bug report Sun Yijiang
2009-12-28 6:48 ` Karl Fogel
2009-12-28 7:41 ` Thierry Volpiatto
2009-12-28 15:58 ` Drew Adams
2009-12-28 21:12 ` Karl Fogel
2009-12-29 18:22 ` Stefan Monnier
2009-12-30 13:37 ` Thierry Volpiatto
2009-12-30 15:28 ` Stefan Monnier
2009-12-30 15:57 ` Drew Adams
2009-12-30 16:26 ` Drew Adams
2009-12-30 17:43 ` Thierry Volpiatto
2009-12-30 18:32 ` Stefan Monnier
2010-01-02 5:05 ` Karl Fogel
2010-01-02 5:15 ` Karl Fogel
2010-01-02 7:03 ` Thierry Volpiatto
2010-01-02 7:26 ` Karl Fogel
2010-01-02 8:15 ` Thierry Volpiatto
2010-01-02 19:17 ` Karl Fogel
2010-01-02 7:51 ` Drew Adams
2010-01-02 8:16 ` Karl Fogel
2010-01-02 8:42 ` Drew Adams
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).