bug#4804: 23.1; bookmark-bmenu-bookmark performances

bug#4804: 23.1; bookmark-bmenu-bookmark performances

[Thierry Volpiatto]

[-- Attachment #1: Type: text/plain, Size: 5109 bytes --]


Please write in English if possible, because the Emacs maintainers
usually do not have translators to read other languages for them.

Your bug report will be posted to the bug-gnu-emacs@gnu.org mailing list,
and to the gnu.emacs.bug news group.

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

Hi,
About bookmark-bmenu-bookmark:

This function work as expected but it is not convenient for the
developper that work on bookmark.el, thus it is inefficient.

bookmark-bmenu-bookmark parse all the menu list with search-* to get the
bookmark name, but to do that, it have to toggle the filenames
visibility if these are toggle on.
That's the problem:

1) run a loop on all menu list to toggle filenames visibility.
2) come back to the original place and get bookmark name with
buffer-substring
3) run again a loop to toggle filenames visibility back.

That's very costly.
If you want one bookmark name, ok but if you run another loop to get
many bookmark names, that will be very long. 

The solution: (It's what i wrote for bookmark+.el.)

1) Create a defvar like `bookmark-latest-sorted-alist'.

,----
| (defvar bookmark-latest-sorted-alist nil)
`----


2) Set this variable to the copy of bookmark-alist obtained in
bookmark-maybe-sort-alist:

,----
| (defun bookmark-maybe-sort-alist ()
|   "Return a sorted copy of `bookmark-alist'.
| If `bookmark-sort-flag' is nil return `bookmark-alist'.
| The sorted copy of `bookmark-alist' is set to `sbookmark-latest-sorted-alist'."
|   (let ((bmk-alist (copy-alist bookmark-alist)))
|     (if bookmark-sort-flag
|         (setq bookmark-latest-sorted-alist
|               (sort bmk-alist #'(lambda (x y) (string-lessp (car x) (car y)))))
|         (setq bookmark-latest-sorted-alist bookmark-alist))))
`----

3) Get the bookmark name from this list with the position in menu list:

,----
| (defun bookmark-bmenu-bookmark ()
|   "Return a string which is bookmark of this line."
|   (let ((pos (- (line-number-at-pos) 3)))
|     (car (nth pos bookmark-latest-sorted-alist))))
`----

That simple and work fine here.
You can now work on menu list of bookmarks without thinking at toggling
filename visibility.(and it's much faster).
That's make easier working in bookmark.el for further modification.

You will find a patch attached.
 

If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
If you would like to further debug the crash, please read the file
/usr/share/emacs/23.1/etc/DEBUG for instructions.


In GNU Emacs 23.1.1 (i686-pc-linux-gnu, GTK+ Version 2.16.5)
 of 2009-07-31 on tux
Windowing system distributor `The X.Org Foundation', version 11.0.10603901
configured using `configure  '--prefix=/usr' '--build=i686-pc-linux-gnu' '--host=i686-pc-linux-gnu' '--mandir=/usr/share/man' '--infodir=/usr/share/info' '--datadir=/usr/share' '--sysconfdir=/etc' '--localstatedir=/var/lib' '--program-suffix=-emacs-23' '--infodir=/usr/share/info/emacs-23' '--with-sound' '--with-x' '--without-toolkit-scroll-bars' '--with-gif' '--with-jpeg' '--with-png' '--with-rsvg' '--with-tiff' '--with-xpm' '--with-xft' '--without-libotf' '--without-m17n-flt' '--with-x-toolkit=gtk' '--without-hesiod' '--without-kerberos' '--without-kerberos5' '--with-gpm' '--with-dbus' 'build_alias=i686-pc-linux-gnu' 'host_alias=i686-pc-linux-gnu' 'CFLAGS=-march=i686 -pipe -O2' 'LDFLAGS=-Wl,-O1''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: C
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: fr_FR.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default-enable-multibyte-characters: t

Major mode: Lisp

Minor modes in effect:
  eldoc-mode: t
  icomplete-mode: t
  icicle-mode: t
  delete-selection-mode: t
  minibuffer-depth-indicate-mode: t
  stumpwm-mode: t
  slime-mode: t
  auto-image-file-mode: t
  partial-completion-mode: t
  show-paren-mode: t
  display-battery-mode: t
  display-time-mode: t
  diff-auto-refine-mode: t
  shell-dirtrack-mode: t
  recentf-mode: t
  savehist-mode: t
  desktop-save-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
<down> <down> C-x r l M-g e m a c s <backspace> <backspace> 
<backspace> <backspace> <backspace> q p <backspace> 
<backspace> <return> q M-x r e p o r t <tab> <down> 
<return>

Recent messages:
bunzip2ing elisp-9.info.bz2...done
bunzip2ing elisp-10.info.bz2...done
bunzip2ing elisp-11.info.bz2...done
Turning ON Icicle mode...done
Desktop lazily opening *info* (1 remaining)...done
Lazy desktop load complete
Collecting symbols...done
Auto-saving...done
Collecting symbols...done
Computing completion candidates...

-- 
A + Thierry Volpiatto
Location: Saint-Cyr-Sur-Mer - France


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: SingleInitial-patchToTip.patch --]
[-- Type: text/x-patch, Size: 3272 bytes --]

##Merge of all patches applied from revision 0
## Initial-patch: New definition of bookmark-bmenu-bookmark by string comparison.(no need to toggle filenames every time).
## patch-r2: Set bookmarkp-latest-sorted-alist to bookmark-alist value when not sorting.
## 
diff --git a/bookmark.el b/bookmark.el
--- a/bookmark.el
+++ b/bookmark.el
@@ -922,14 +922,15 @@
        (setq bookmarks-already-loaded t)))
 
 
+(defvar bookmarkp-latest-sorted-alist nil)
 (defun bookmark-maybe-sort-alist ()
   ;;Return the bookmark-alist for display.  If the bookmark-sort-flag
   ;;is non-nil, then return a sorted copy of the alist.
-  (if bookmark-sort-flag
-      (sort (copy-alist bookmark-alist)
-            (function
-             (lambda (x y) (string-lessp (car x) (car y)))))
-    bookmark-alist))
+  (let ((bmk-alist (copy-alist bookmark-alist)))
+    (if bookmark-sort-flag
+        (setq bookmarkp-latest-sorted-alist
+              (sort bmk-alist #'(lambda (x y) (string-lessp (car x) (car y)))))
+        (setq bookmarkp-latest-sorted-alist bookmark-alist))))
 
 
 (defvar bookmark-after-jump-hook nil
@@ -1670,28 +1671,9 @@
 
 
 (defun bookmark-bmenu-bookmark ()
-  ;; return a string which is bookmark of this line.
-  (if (bookmark-bmenu-check-position)
-      (save-excursion
-        (save-window-excursion
-          (goto-char (point-min))
-          (search-forward "Bookmark")
-          (backward-word 1)
-          (setq bookmark-bmenu-bookmark-column (current-column)))))
-  (if bookmark-bmenu-toggle-filenames
-      (bookmark-bmenu-hide-filenames))
-  (save-excursion
-    (save-window-excursion
-      (beginning-of-line)
-      (forward-char bookmark-bmenu-bookmark-column)
-      (prog1
-          (buffer-substring-no-properties (point)
-                            (progn
-                              (end-of-line)
-                              (point)))
-        ;; well, this is certainly crystal-clear:
-        (if bookmark-bmenu-toggle-filenames
-            (bookmark-bmenu-toggle-filenames t))))))
+  "Return a string which is bookmark of this line."
+  (let ((pos (- (line-number-at-pos) 3)))
+    (car (nth pos bookmarkp-latest-sorted-alist))))
 
 
 (defun bookmark-show-annotation (bookmark)
@@ -1936,8 +1918,7 @@
   "Delete bookmarks marked with \\<Buffer-menu-mode-map>\\[Buffer-menu-delete] commands."
   (interactive)
   (message "Deleting bookmarks...")
-  (let ((hide-em bookmark-bmenu-toggle-filenames)
-        (o-point  (point))
+  (let ((o-point  (point))
         (o-str    (save-excursion
                     (beginning-of-line)
                     (if (looking-at "^D")
@@ -1946,16 +1927,11 @@
                        (point)
                        (progn (end-of-line) (point))))))
         (o-col     (current-column)))
-    (if hide-em (bookmark-bmenu-hide-filenames))
-    (setq bookmark-bmenu-toggle-filenames nil)
     (goto-char (point-min))
     (forward-line 1)
     (while (re-search-forward "^D" (point-max) t)
       (bookmark-delete (bookmark-bmenu-bookmark) t)) ; pass BATCH arg
     (bookmark-bmenu-list)
-    (setq bookmark-bmenu-toggle-filenames hide-em)
-    (if bookmark-bmenu-toggle-filenames
-        (bookmark-bmenu-toggle-filenames t))
     (if o-str
         (progn
           (goto-char (point-min))

bug#4804: 23.1; bookmark-bmenu-bookmark performances

[Stefan Monnier]

> 1) run a loop on all menu list to toggle filenames visibility.
> 2) come back to the original place and get bookmark name with
> buffer-substring
> 3) run again a loop to toggle filenames visibility back.
> That's very costly.

Agreed, it's costly, brittle, and ugly.

But your solution relies on a correspondance between
bookmark-latest-sorted-alist and the buffer's content, even though this
correspondance is nowhere enforced (i.e. future changes to the code are
likely to break it, or manual buffer modifications could also break it).

Much easier and more robust would be to put the relevant data directly
in the buffer in the form of text-properties, so it can be extracted
directly via get-text-property regardless of whether the filenames
are hidden.


        Stefan

bug#4804: 23.1; bookmark-bmenu-bookmark performances

[Stefan Kangas]

tags 4804 fixed
close 4804 23.2
quit

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> 1) run a loop on all menu list to toggle filenames visibility.
>> 2) come back to the original place and get bookmark name with
>> buffer-substring
>> 3) run again a loop to toggle filenames visibility back.
>> That's very costly.
>
> Agreed, it's costly, brittle, and ugly.
>
> But your solution relies on a correspondance between
> bookmark-latest-sorted-alist and the buffer's content, even though this
> correspondance is nowhere enforced (i.e. future changes to the code are
> likely to break it, or manual buffer modifications could also break it).
>
> Much easier and more robust would be to put the relevant data directly
> in the buffer in the form of text-properties, so it can be extracted
> directly via get-text-property regardless of whether the filenames
> are hidden.
>
>
>         Stefan

This was subsequently implemented by Stefan Monnier:

fc9d6ad645ab0332811bcd7b79341f68ddd958e8
2009-11-21 06:43:45 +0000
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fc9d6ad645ab0332811bcd7b79341f68ddd958e8

I'm therefore closing this bug report.

Thanks,
Stefan Kangas