Fix previous-button to work with connected buttons

Fix previous-button to work with connected buttons

[Diane Murray]

`previous-button' doesn't work correctly when two buttons inserted
with `insert-button' are right next to each other.  Instead of
returning the previous button, it skips it and returns the one before
it, even though there's no skip property set.  The following patch
fixes this.

Neither `previous-button' nor `next-button' work well with neighboring
buttons inserted with `insert-text-button' - this patch is *not* able
to solve that problem.


2007-03-11  Diane Murray  <disumu@x3y2z1.net>

	* button.el (previous-button): Fixed to work correctly when two
	overlay buttons are next to each other.


*** button.el	24 Jan 2007 13:01:15 +0100	1.22
--- button.el	11 Mar 2007 00:20:28 +0100	
***************
*** 371,377 ****
    (unless count-current
      (setq pos (previous-single-char-property-change pos 'button)))
    (and (> pos (point-min))
!        (or (button-at (1- pos))
  	   ;; We must have originally been on a button, and are now in
  	   ;; the inter-button space.  Recurse to find a button.
  	   (previous-button pos))))
--- 371,377 ----
    (unless count-current
      (setq pos (previous-single-char-property-change pos 'button)))
    (and (> pos (point-min))
!        (or (button-at pos)
  	   ;; We must have originally been on a button, and are now in
  	   ;; the inter-button space.  Recurse to find a button.
  	   (previous-button pos))))

Re: Fix previous-button to work with connected buttons

[Stefan Monnier]

> Neither `previous-button' nor `next-button' work well with neighboring
> buttons inserted with `insert-text-button' - this patch is *not* able
> to solve that problem.

I think the patch below should provide a general fix.
Can you test it?


        Stefan


--- orig/lisp/button.el
+++ mod/lisp/button.el
@@ -89,10 +89,6 @@
 ;; Prevent insertions adjacent to the text-property buttons from
 ;; inheriting its properties.
 (put 'default-button 'rear-nonsticky t)
-;; Text property buttons don't have a `button' property of their own, so
-;; they inherit this.
-(put 'default-button 'button t)
-
 ;; A `category-symbol' property for the default button type
 (put 'button 'button-category-symbol 'default-button)
 
@@ -316,7 +312,11 @@
       (setcar (cdr type-entry)
 	      (button-category-symbol (car (cdr type-entry))))))
   ;; Now add all the text properties at once
-  (add-text-properties beg end properties)
+  (add-text-properties beg end
+                       ;; Each button should have a different `button'
+                       ;; property so that next-single-property-change can
+                       ;; detect boundaries reliably.
+                       (cons 'button (cons (copy-marker beg t) properties)))
   ;; Return something that can be used to get at the button.
   beg)
 
@@ -345,11 +345,7 @@
 
 (defun button-at (pos)
   "Return the button at position POS in the current buffer, or nil."
-  (let ((button (get-char-property pos 'button)))
-    (if (or (overlayp button) (null button))
-	button
-      ;; Must be a text-property button; return a marker pointing to it.
-      (copy-marker pos t))))
+  (get-char-property pos 'button))
 
 (defun next-button (pos &optional count-current)
   "Return the next button after position POS in the current buffer.
@@ -453,5 +449,5 @@
 
 (provide 'button)
 
-;;; arch-tag: 5f2c7627-413b-4097-b282-630f89d9c5e9
+;; arch-tag: 5f2c7627-413b-4097-b282-630f89d9c5e9
 ;;; button.el ends here

Re: Fix previous-button to work with connected buttons

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Neither `previous-button' nor `next-button' work well with neighboring
>> buttons inserted with `insert-text-button' - this patch is *not* able
>> to solve that problem.
>
> I think the patch below should provide a general fix.

Please don't do that -- one of the goals of buttons is to work
reasonably well if there are _lots_ of them, and that was the main
reason for supporting both "overlay" and "text" buttons.  If you have
lots of buttons, creating a unique marker for each button has nasty
results (I've tested it).

[If adjacent buttons are important, I'd rather do something that only
affects that case.  For instance, I suppose a hacky approach might be to
have two different button properties, like `button' and `button2', and
use `button2' if inserting just before another button that uses
`button']

Thanks,

-Miles

-- 
Saa, shall we dance?  (from a dance-class advertisement)

Re: Fix previous-button to work with connected buttons

[Stefan Monnier]

>>> Neither `previous-button' nor `next-button' work well with neighboring
>>> buttons inserted with `insert-text-button' - this patch is *not* able
>>> to solve that problem.
>> 
>> I think the patch below should provide a general fix.

> Please don't do that -- one of the goals of buttons is to work
> reasonably well if there are _lots_ of them, and that was the main
> reason for supporting both "overlay" and "text" buttons.  If you have
> lots of buttons, creating a unique marker for each button has nasty
> results (I've tested it).

Fair enough.  Also if the button is copied to a string and to other buffers,
the resulting marker ends up pointing elsewhere which may surprise other
pieces of code (although I couldn't find any).

How 'bout the patch below, then?


        Stefan


--- button.el	26 jan 2007 23:51:22 -0500	1.22
+++ button.el	11 mar 2007 00:58:33 -0500	
@@ -89,9 +89,6 @@
 ;; Prevent insertions adjacent to the text-property buttons from
 ;; inheriting its properties.
 (put 'default-button 'rear-nonsticky t)
-;; Text property buttons don't have a `button' property of their own, so
-;; they inherit this.
-(put 'default-button 'button t)
 
 ;; A `category-symbol' property for the default button type
 (put 'button 'button-category-symbol 'default-button)
@@ -316,7 +313,11 @@
       (setcar (cdr type-entry)
 	      (button-category-symbol (car (cdr type-entry))))))
   ;; Now add all the text properties at once
-  (add-text-properties beg end properties)
+  (add-text-properties beg end
+                       ;; Each button should have a different `button'
+                       ;; property so that next-single-property-change can
+                       ;; detect boundaries reliably.
+                       (cons 'button (cons (list t) properties)))
   ;; Return something that can be used to get at the button.
   beg)

Re: Fix previous-button to work with connected buttons

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> How 'bout the patch below, then?
> +  (add-text-properties beg end
> +                       ;; Each button should have a different `button'
> +                       ;; property so that next-single-property-change can
> +                       ;; detect boundaries reliably.
> +                       (cons 'button (cons (list t) properties)))

Hmm.  Any reason to waste a cons-cell?  I imagine something like
(cons 'button (cons (setq button-counter (1+ button-counter)) properties))
would work as well.

-Miles

-- 
The key to happiness
  is having dreams.	[from a fortune cookie]

Re: Fix previous-button to work with connected buttons

[David Kastrup]

Miles Bader <miles@gnu.org> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> How 'bout the patch below, then?
>> +  (add-text-properties beg end
>> +                       ;; Each button should have a different `button'
>> +                       ;; property so that next-single-property-change can
>> +                       ;; detect boundaries reliably.
>> +                       (cons 'button (cons (list t) properties)))
>
> Hmm.  Any reason to waste a cons-cell?  I imagine something like
> (cons 'button (cons (setq button-counter (1+ button-counter)) properties))
                ^^^^^^
> would work as well.

How about (cons 'button (cons (car properties) (cdr properties)))?

That provides an EQ-unique value without all the added fluff.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Fix previous-button to work with connected buttons

[Miles Bader]

David Kastrup <dak@gnu.org> writes:
>> Hmm.  Any reason to waste a cons-cell?  I imagine something like
>> (cons 'button (cons (setq button-counter (1+ button-counter)) properties))
>                 ^^^^^^
>> would work as well.
>
> How about (cons 'button (cons (car properties) (cdr properties)))?
>
> That provides an EQ-unique value without all the added fluff.

It's also an invalid property-list... :-/

-Miles
-- 
Fast, small, soon; pick any 2.

Re: Fix previous-button to work with connected buttons

[David Kastrup]

Miles Bader <miles@gnu.org> writes:

> David Kastrup <dak@gnu.org> writes:
>>> Hmm.  Any reason to waste a cons-cell?  I imagine something like
>>> (cons 'button (cons (setq button-counter (1+ button-counter)) properties))
>>                 ^^^^^^
>>> would work as well.
>>
>> How about (cons 'button (cons (car properties) (cdr properties)))?
>>
>> That provides an EQ-unique value without all the added fluff.
>
> It's also an invalid property-list... :-/

Ok, I did not understand the fine points of just _what_ was supposed
to be part of what.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Fix previous-button to work with connected buttons

[Stefan Monnier]

>> How 'bout the patch below, then?
>> +  (add-text-properties beg end
>> +                       ;; Each button should have a different `button'
>> +                       ;; property so that next-single-property-change can
>> +                       ;; detect boundaries reliably.
>> +                       (cons 'button (cons (list t) properties)))
> Hmm.  Any reason to waste a cons-cell?

Because it's simpler and 100% reliable, and the waste is negligible (think
of the amount of consing needed for the total list of properties, plus the
amount of consing needed for the data-structure that keeps track of the
text-properties, ...).

> I imagine something like
> (cons 'button (cons (setq button-counter (1+ button-counter)) properties))
> would work as well.

Sure.  Except for wraparounds, of course.  Not that they're likely.


        Stefan

Re: Fix previous-button to work with connected buttons

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I imagine something like
>> (cons 'button (cons (setq button-counter (1+ button-counter)) properties))
>> would work as well.
>
> Sure.  Except for wraparounds, of course.  Not that they're likely.

A simple wrap-around wouldn't cause any problem either -- really it only
matters that _adjacent_ buttons (with no intervening text between them)
have different values.

-miles
-- 
(\(\
(^.^)
(")")
*This is the cute bunny virus, please copy this into your sig so it can spread.

Re: Fix previous-button to work with connected buttons

[Diane Murray]

The latest changes to button.el have added a new distinct button
property which makes it possible to correctly determine that
neighboring buttons are different and to navigate through them even
when they are right next to each other.  There is still one little
bug, however, which I corrected in a patch I sent in my original email
- the problem it fixes is that any previous adjacent button is
skipped.  Please see the patch below with a new diff from the latest
revision.


2007-04-01  Diane Murray  <disumu@x3y2z1.net>

	* button.el (previous-button): Fixed to work correctly when two
	buttons are next to each other.


*** button.el	28 Mar 2007 03:20:30 -0000	1.23
--- button.el	1 Apr 2007 02:21:12 -0000
***************
*** 372,378 ****
    (unless count-current
      (setq pos (previous-single-char-property-change pos 'button)))
    (and (> pos (point-min))
!        (or (button-at (1- pos))
  	   ;; We must have originally been on a button, and are now in
  	   ;; the inter-button space.  Recurse to find a button.
  	   (previous-button pos))))
--- 372,378 ----
    (unless count-current
      (setq pos (previous-single-char-property-change pos 'button)))
    (and (> pos (point-min))
!        (or (button-at pos)
  	   ;; We must have originally been on a button, and are now in
  	   ;; the inter-button space.  Recurse to find a button.
  	   (previous-button pos))))

Re: Fix previous-button to work with connected buttons

[Richard Stallman]

Would someone please install this, if it is correct, and then ack?

Date: Sun, 01 Apr 2007 04:28:10 +0200
From: Diane Murray <disumu@x3y2z1.net>
To: emacs-devel@gnu.org
Subject: Re: Fix previous-button to work with connected buttons

The latest changes to button.el have added a new distinct button
property which makes it possible to correctly determine that
neighboring buttons are different and to navigate through them even
when they are right next to each other.  There is still one little
bug, however, which I corrected in a patch I sent in my original email
- the problem it fixes is that any previous adjacent button is
skipped.  Please see the patch below with a new diff from the latest
revision.


2007-04-01  Diane Murray  <disumu@x3y2z1.net>

	* button.el (previous-button): Fixed to work correctly when two
	buttons are next to each other.


*** button.el	28 Mar 2007 03:20:30 -0000	1.23
--- button.el	1 Apr 2007 02:21:12 -0000
***************
*** 372,378 ****
    (unless count-current
      (setq pos (previous-single-char-property-change pos 'button)))
    (and (> pos (point-min))
!        (or (button-at (1- pos))
  	   ;; We must have originally been on a button, and are now in
  	   ;; the inter-button space.  Recurse to find a button.
  	   (previous-button pos))))
--- 372,378 ----
    (unless count-current
      (setq pos (previous-single-char-property-change pos 'button)))
    (and (> pos (point-min))
!        (or (button-at pos)
  	   ;; We must have originally been on a button, and are now in
  	   ;; the inter-button space.  Recurse to find a button.
  	   (previous-button pos))))


_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Fix previous-button to work with connected buttons

[Chong Yidong]

Diane Murray <disumu@x3y2z1.net> writes:

> The latest changes to button.el have added a new distinct button
> property which makes it possible to correctly determine that
> neighboring buttons are different and to navigate through them even
> when they are right next to each other.  There is still one little
> bug, however, which I corrected in a patch I sent in my original email
> - the problem it fixes is that any previous adjacent button is
> skipped.  Please see the patch below with a new diff from the latest
> revision.

Thanks for the report.

The patch you suggested didn't give good results.  I've checked in a
different patch to handle this case.