Re: [elpa] externals/hyperbole c501027 2/2: Fix set-buffer byte-compiler warnings; remove outdated references

Re: [elpa] externals/hyperbole c501027 2/2: Fix set-buffer byte-compiler warnings; remove outdated references

[Stefan Monnier]

>  	(save-excursion
> -	  (set-buffer (if (bufferp loc) loc (find-file-noselect loc)))
> -	  (when (ibut:to (ibut:key but-sym))
> -	    (let (buffer-read-only)
> -	      (if (< (point) start)
> -		  ;; Find beginning of button named label delimiter and delete
> -		  ;; from there.
> -		  (progn (goto-char (- (point) (length ibut:label-start)))
> -			 (delete-region (point) end))
> -		;; No label, just delete delimited ibutton text.
> -		(delete-region start end))
> -	      (when (looking-at "[ \t]*\r?\n")
> -		(delete-region (point) (match-end 0)))
> -	      (run-hooks 'ibut-delete-hook))))
> +	  (with-current-buffer (if (bufferp loc) loc (find-file-noselect loc))
> +	    (when (ibut:to (ibut:key but-sym))
> +	      (let (buffer-read-only)
> +		(if (< (point) start)
> +		    ;; Find beginning of button named label delimiter and delete
> +		    ;; from there.
> +		    (progn (goto-char (- (point) (length ibut:label-start)))
> +			   (delete-region (point) end))
> +		  ;; No label, just delete delimited ibutton text.
> +		  (delete-region start end))
> +		(when (looking-at "[ \t]*\r?\n")
> +		  (delete-region (point) (match-end 0)))
> +		(run-hooks 'ibut-delete-hook)))))
>  	but-sym))))
>  
>  (defun    ibut:get (&optional lbl-key buffer key-src)
> diff --git a/hui-window.el b/hui-window.el
> index 654c035..796770e 100644
> --- a/hui-window.el
> +++ b/hui-window.el
> @@ -311,12 +311,12 @@ part of InfoDock and not a part of Hyperbole)."
>    "Return t iff there is a non-empty active region in buffer of the last Smart Mouse Key release."
>    (when (setq hkey-value (if assist-flag assist-key-depress-prev-point action-key-depress-prev-point))
>      (save-excursion
> -      (set-buffer (marker-buffer hkey-value))
> -      ;; Store and goto any prior value of point from the region
> -      ;; prior to the Smart Key depress, so we can return to it later.
> -      (and (goto-char hkey-value)
> -	   (hmouse-save-region)
> -	   t))))
> +      (with-current-buffer (marker-buffer hkey-value)
> +	;; Store and goto any prior value of point from the region
> +	;; prior to the Smart Key depress, so we can return to it later.
> +	(and (goto-char hkey-value)
> +	     (hmouse-save-region)
> +	     t)))))

These two don't make much sense: `save-excursion` only saves the
position of point in the current buffer, so

    (save-excursion (with-current-buffer FOO ...))

is useless in one of two ways:
- FOO is already the current buffer, so `with-current-buffer`
  (previously `set-buffer`) does nothing.
- FOO is a different buffer, so the buffer-position saved&restored by
  `save-excursion` is in a buffer that's not affected by `...`.

If you want to combine the two, then you should use

    (with-current-buffer FOO (save-excursion ...))

Tho if we presume that the `set-buffer` in the previous code did do
something useful (i.e. selected a different buffer), then there's a good
chance that the `save-excursion` has never done its intended job and can
be removed.


        Stefan

Re: [elpa] externals/hyperbole c501027 2/2: Fix set-buffer byte-compiler warnings; remove outdated references

[Robert Weiner]

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

Eli and I exchanged messages about this on the emacs-devel list and settled
on the outer save-excursion as you first wrote below since it is indeed the
original buffer whose point we want to save which may be manipulated by
some subroutine deep within the with-current-buffer that changes the buffer
back to the original.  Thanks as always for the pointers though.  -- Bob

On Mon, May 10, 2021 at 10:00 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> >       (save-excursion
> > -       (set-buffer (if (bufferp loc) loc (find-file-noselect loc)))
> > -       (when (ibut:to (ibut:key but-sym))
> > -         (let (buffer-read-only)
> > -           (if (< (point) start)
> > -               ;; Find beginning of button named label delimiter and
> delete
> > -               ;; from there.
> > -               (progn (goto-char (- (point) (length ibut:label-start)))
> > -                      (delete-region (point) end))
> > -             ;; No label, just delete delimited ibutton text.
> > -             (delete-region start end))
> > -           (when (looking-at "[ \t]*\r?\n")
> > -             (delete-region (point) (match-end 0)))
> > -           (run-hooks 'ibut-delete-hook))))
> > +       (with-current-buffer (if (bufferp loc) loc (find-file-noselect
> loc))
> > +         (when (ibut:to (ibut:key but-sym))
> > +           (let (buffer-read-only)
> > +             (if (< (point) start)
> > +                 ;; Find beginning of button named label delimiter and
> delete
> > +                 ;; from there.
> > +                 (progn (goto-char (- (point) (length
> ibut:label-start)))
> > +                        (delete-region (point) end))
> > +               ;; No label, just delete delimited ibutton text.
> > +               (delete-region start end))
> > +             (when (looking-at "[ \t]*\r?\n")
> > +               (delete-region (point) (match-end 0)))
> > +             (run-hooks 'ibut-delete-hook)))))
> >       but-sym))))
> >
> >  (defun    ibut:get (&optional lbl-key buffer key-src)
> > diff --git a/hui-window.el b/hui-window.el
> > index 654c035..796770e 100644
> > --- a/hui-window.el
> > +++ b/hui-window.el
> > @@ -311,12 +311,12 @@ part of InfoDock and not a part of Hyperbole)."
> >    "Return t iff there is a non-empty active region in buffer of the
> last Smart Mouse Key release."
> >    (when (setq hkey-value (if assist-flag assist-key-depress-prev-point
> action-key-depress-prev-point))
> >      (save-excursion
> > -      (set-buffer (marker-buffer hkey-value))
> > -      ;; Store and goto any prior value of point from the region
> > -      ;; prior to the Smart Key depress, so we can return to it later.
> > -      (and (goto-char hkey-value)
> > -        (hmouse-save-region)
> > -        t))))
> > +      (with-current-buffer (marker-buffer hkey-value)
> > +     ;; Store and goto any prior value of point from the region
> > +     ;; prior to the Smart Key depress, so we can return to it later.
> > +     (and (goto-char hkey-value)
> > +          (hmouse-save-region)
> > +          t)))))
>
> These two don't make much sense: `save-excursion` only saves the
> position of point in the current buffer, so
>
>     (save-excursion (with-current-buffer FOO ...))
>
> is useless in one of two ways:
> - FOO is already the current buffer, so `with-current-buffer`
>   (previously `set-buffer`) does nothing.
> - FOO is a different buffer, so the buffer-position saved&restored by
>   `save-excursion` is in a buffer that's not affected by `...`.
>
> If you want to combine the two, then you should use
>
>     (with-current-buffer FOO (save-excursion ...))
>
> Tho if we presume that the `set-buffer` in the previous code did do
> something useful (i.e. selected a different buffer), then there's a good
> chance that the `save-excursion` has never done its intended job and can
> be removed.
>
>
>         Stefan
>
>

[-- Attachment #2: Type: text/html, Size: 4963 bytes --]

Re: [elpa] externals/hyperbole c501027 2/2: Fix set-buffer byte-compiler warnings; remove outdated references

[Stefan Monnier]

> Eli and I exchanged messages about this on the emacs-devel list and settled
> on the outer save-excursion as you first wrote below since it is indeed the
> original buffer whose point we want to save which may be manipulated by
> some subroutine deep within the with-current-buffer that changes the buffer
> back to the original.  Thanks as always for the pointers though.  -- Bob

That seems to point at a very unusual arrangement which you might want
(in the longer term) to reconsider, e.g. by moving the `save-excursion`
to the "deep within" place that actually moves the point in the "outer
buffer".

In the mean time, adding a comment explaining what's going on
(e.g. giving an example scenario where point would be moved unexpectedly
in the outer buffer)would go a long way.

Similarly for the

  (save-excursion
    (with-current-buffer (find-file-noselect (expand-file-name hbmap:filename hbmap:dir-user))
      (save-excursion

in `gbut:ebut-program` because without an explanatory comment, this looks
like the kind of code you get when the author just "added
`save-excursion`s a bit at random until point stopped moving" ;-)


        Stefan

Re: [elpa] externals/hyperbole c501027 2/2: Fix set-buffer byte-compiler warnings; remove outdated references

[Jean Louis]

* Stefan Monnier <monnier@iro.umontreal.ca> [2021-05-10 17:01]:
> These two don't make much sense: `save-excursion` only saves the
> position of point in the current buffer, so
> 
>     (save-excursion (with-current-buffer FOO ...))
> 
> is useless in one of two ways:
> - FOO is already the current buffer, so `with-current-buffer`
>   (previously `set-buffer`) does nothing.
> - FOO is a different buffer, so the buffer-position saved&restored by
>   `save-excursion` is in a buffer that's not affected by `...`.

Thank you, I totally understood this one, though I am unrelated...


-- 
Jean

Take action in Free Software Foundation campaigns:
https://www.fsf.org/campaigns

Sign an open letter in support of Richard M. Stallman
https://stallmansupport.org/
https://rms-support-letter.github.io/