bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Drew Adams]

Prior to Emacs 23, the delsel.el code that handles a yank had this code:

;; Before a yank command, make sure we don't yank the
;; head of the kill-ring that really comes from the
;; currently active region we are going to delete.
;; That would make yank a no-op.
(when (and (string= (buffer-substring-no-properties (point) (mark))
                    (car kill-ring)))
  (current-kill 1))

That code dates from the creation of delsel.el.  In Emacs 23, these
two sexps were appended to the `and' conditions, which completely
changes the meaning/behavior (though the comment was not modified):

 (fboundp 'mouse-region-match)
 (mouse-region-match)

This changed the behavior so that the yanking point is now rotated only
when the mouse was used to set the region.  This makes no sense to me,
but perhaps I am missing something.  Does this make sense to you?
If it does make sense, then at least the comment should be updated to
reflect the current behavior.


---

BTW, I also think it would be better to use `while' instead of `when'
in the code:

(let ((tail  kill-ring))
  (while (and tail  (string= (buffer-substring-no-properties (point) (mark))
                             (car tail))
              (fboundp 'mouse-region-match) ; ????
              (mouse-region-match)) ; ????
    (current-kill 1)
    (setq tail  (cdr tail))))


In GNU Emacs 24.4.50.1 (i686-pc-mingw32)
 of 2014-06-28 on ODIEONE
Bzr revision: 117431 rgm@gnu.org-20140628015517-eku6hj8mpgcvfnso
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/snapshot/trunk
 --enable-checking=yes,glyphs 'CFLAGS=-O0 -g3'
 LDFLAGS=-Lc:/Devel/emacs/lib 'CPPFLAGS=-DGC_MCHECK=1
 -Ic:/Devel/emacs/include''

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Drew Adams]

ping

Could someone else please take a look at this?
Thx.

> Sent: Wednesday, July 23, 2014 11:03 AM


> Prior to Emacs 23, the delsel.el code that handles a yank had this code:
> 
> ;; Before a yank command, make sure we don't yank the
> ;; head of the kill-ring that really comes from the
> ;; currently active region we are going to delete.
> ;; That would make yank a no-op.
> (when (and (string= (buffer-substring-no-properties (point) (mark))
>                     (car kill-ring)))
>   (current-kill 1))
> 
> That code dates from the creation of delsel.el.  In Emacs 23, these
> two sexps were appended to the `and' conditions, which completely
> changes the meaning/behavior (though the comment was not modified):
> 
>  (fboundp 'mouse-region-match)
>  (mouse-region-match)
> 
> This changed the behavior so that the yanking point is now rotated only
> when the mouse was used to set the region.  This makes no sense to me,
> but perhaps I am missing something.  Does this make sense to you?
> If it does make sense, then at least the comment should be updated to
> reflect the current behavior.
> 
> 
> ---
> 
> BTW, I also think it would be better to use `while' instead of `when'
> in the code:
> 
> (let ((tail  kill-ring))
>   (while (and tail  (string= (buffer-substring-no-properties (point)
> (mark))
>                              (car tail))
>               (fboundp 'mouse-region-match) ; ????
>               (mouse-region-match)) ; ????
>     (current-kill 1)
>     (setq tail  (cdr tail))))
> 
> 
> In GNU Emacs 24.4.50.1 (i686-pc-mingw32)
>  of 2014-06-28 on ODIEONE
> Bzr revision: 117431 rgm@gnu.org-20140628015517-eku6hj8mpgcvfnso
> Windowing system distributor `Microsoft Corp.', version 6.1.7601
> Configured using:
>  `configure --prefix=/c/Devel/emacs/snapshot/trunk
>  --enable-checking=yes,glyphs 'CFLAGS=-O0 -g3'
>  LDFLAGS=-Lc:/Devel/emacs/lib 'CPPFLAGS=-DGC_MCHECK=1
>  -Ic:/Devel/emacs/include''
> 
> 
> 

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Drew Adams]

ping

> Sent: Tuesday, April 3, 2018 2:06 PM
> ping
> Could someone else please take a look at this?
> Thx.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Eli Zaretskii]

> Date: Wed, 19 Sep 2018 10:51:39 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> 
> ping
> 
> > Sent: Tuesday, April 3, 2018 2:06 PM
> > ping
> > Could someone else please take a look at this?
> > Thx.

That change was made due to problems described in this thread:

  http://lists.gnu.org/archive/html/emacs-devel/2007-06/msg01642.html

FWIW, I cannot reproduce the original problem reported by Juri in that
thread, in Emacs 22.1, which according to my calculations was released
before the change.  So I cannot dig deeper into the issue, but my
first thought was that this code needs to be rethought/augmented due
to changes in Emacs 24 regarding selections, since currently the
default is not to copy to the kill-ring the region selected by
dragging the mouse.

I also don't think I understand your report, Drew.  What exactly is
the problem you are complaining about?  Surely, the problem is not the
code, but its effect; however, all you say about the effect is that
"yanking point is rotated only when the mouse was used".  I don't
understand what "rotating yanking point" means, let alone why it is
problematic.

I hope Juri, who made that change, will be able to investigate some
more.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Drew Adams]

> That change was made due to problems described in this thread:
> 
>   https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__lists.gnu.org_archive_html_emacs-2Ddevel_2007-
> 2D06_msg01642.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMU
> B65eapI_JnE&r=kI3P6ljGv6CTHIKju0jqInF6AOwMCYRDQUmqX22rJ98&m=1Sma
> MGY0ePhv2v_dZNfjfbgjYaW_usF7c6AeSBPND4A&s=tjJCnI84nM3eLwqFSz2ImzZ
> U1RBUgqh9dbJs3SvCK_w&e=
> 
> FWIW, I cannot reproduce the original problem reported by Juri in that
> thread, in Emacs 22.1, which according to my calculations was released
> before the change.  So I cannot dig deeper into the issue, but my
> first thought was that this code needs to be rethought/augmented due
> to changes in Emacs 24 regarding selections, since currently the
> default is not to copy to the kill-ring the region selected by
> dragging the mouse.
> 
> I also don't think I understand your report, Drew.  What exactly is
> the problem you are complaining about?  Surely, the problem is not the
> code, but its effect; however, all you say about the effect is that
> "yanking point is rotated only when the mouse was used".  I don't
> understand what "rotating yanking point" means, let alone why it is
> problematic.
> 
> I hope Juri, who made that change, will be able to investigate some
> more.

I have my own code for `delete-selection-mode' stuff, including my
own version of ` delete-selection-helper'. I added the two lines that
were added to the vanilla version for reference, but I commented
them out. I see no need for them - no problem without them. And
I think it doesn't make sense to treat the mouse differently here.

 (while (and tail  (string= (buffer-substring-no-properties (point) (mark))
           ;; Vanilla Emacs has this, which seems very wrong.
           ;; See Emacs bug #18090.
           ;;;   (fboundp 'mouse-region-match)
           ;;;   (mouse-region-match)
           )

I can't add more than that, I'm afraid. I don't see the bug that Juri
reports, even though the code I use does not have those two
commented lines.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Juri Linkov]

> That change was made due to problems described in this thread:
>
>   http://lists.gnu.org/archive/html/emacs-devel/2007-06/msg01642.html
>
> FWIW, I cannot reproduce the original problem reported by Juri in that
> thread, in Emacs 22.1, which according to my calculations was released
> before the change.  So I cannot dig deeper into the issue, but my
> first thought was that this code needs to be rethought/augmented due
> to changes in Emacs 24 regarding selections, since currently the
> default is not to copy to the kill-ring the region selected by
> dragging the mouse.

I see that mouse operations are not supported by delete-selection-mode
anyway - yanking with <mouse-2> doesn't replace the selected region,
so the whole

	     (when (and (string= (buffer-substring-no-properties
				  (point) (mark))
				 (car kill-ring))
			(fboundp 'mouse-region-match)
			(mouse-region-match))
	       (current-kill 1))

could be deleted altogether.  Otherwise, if there is a wish to support
mouse yanking, a separate enhancement request could be created.

> I also don't think I understand your report, Drew.  What exactly is
> the problem you are complaining about?  Surely, the problem is not the
> code, but its effect; however, all you say about the effect is that
> "yanking point is rotated only when the mouse was used".  I don't
> understand what "rotating yanking point" means, let alone why it is
> problematic.

I don't understand this bug report too - in fact, there is no bug,
the purpose of the old change was to remove gratuitous optimization
that breaks keyboard macros (the result of yanking should not depend
on differences between old and new text), and nobody complained
during last 10+ years.

But if Drew wants such optimization, then a new customizable option
could be added.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Drew Adams]

> I see that mouse operations are not supported by delete-selection-mode
> anyway - yanking with <mouse-2> doesn't replace the selected region,

Huh? I kill some text (or copy it to the kill-ring). Then I select some
other text with the mouse. Then I type or I yank. The typed or
yanked text replaces the mouse-selected text. What am I missing?

> so the whole
> 	     (when (and (string= (buffer-substring-no-properties
> 				  (point) (mark)) (car kill-ring))
> 			(fboundp 'mouse-region-match)
> 			(mouse-region-match))
> 	       (current-kill 1))
> could be deleted altogether.  

Why would we do that? That's been there forever - except for the
mouse-region-match part that you added (subject of this report).

> Otherwise, if there is a wish to support
> mouse yanking, a separate enhancement request could be created.

Why do you think that clause is about mouse yanking? It's about
using a command that has `yank' as the value of its `delete-selection'
property.

> I don't understand this bug report too - in fact, there is no bug,
> the purpose of the old change was to remove gratuitous optimization
> that breaks keyboard macros (the result of yanking should not depend
> on differences between old and new text), 

Sorry, but I don't understand that. What old change? What
optimization? What macro breakage (example)? What old and
new text? No idea what you're saying, there.

> and nobody complained during last 10+ years.

I filed this bug report 4 years ago. What kinds of complaints are
you expecting?

> But if Drew wants such optimization, then a new customizable option
> could be added.

I don't know what optimization you're talking about; sorry.

The comment that introduces the code that you think is
extra and should be deleted is this:

;; Before a yank command, make sure we don't yank the
;; head of the kill-ring that really comes from the
;; currently active region we are going to delete.
;; That would make yank a no-op.

That comment has been there since at least Emacs 20!

It says that the purpose of this code is to prevent just yanking
the same text that will be deleted. That's the entire purpose
of property-value `yank'. This is not an optimization. It's
precisely so you _can_ yank to replace the selection. Without
this code you cannot - you end up replacing the selection
with its own text.

The part of the code that you want to remove is the ONLY
code that has been present forever for the `yank' condition.
Remove that and you'll remove the use of property-value
`yank'.

Clearly I'm not understanding you. Please try to clarify. I hope
this is just a case of you or me missing something, and not one
of real disagreement.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Juri Linkov]

>> I see that mouse operations are not supported by delete-selection-mode
>> anyway - yanking with <mouse-2> doesn't replace the selected region,
>
> Huh? I kill some text (or copy it to the kill-ring). Then I select some
> other text with the mouse. Then I type or I yank. The typed or
> yanked text replaces the mouse-selected text. What am I missing?

You are missing the <mouse-2> part above.

>> so the whole
>> 	     (when (and (string= (buffer-substring-no-properties
>> 				  (point) (mark)) (car kill-ring))
>> 			(fboundp 'mouse-region-match)
>> 			(mouse-region-match))
>> 	       (current-kill 1))
>> could be deleted altogether.
>
> Why would we do that? That's been there forever - except for the
> mouse-region-match part that you added (subject of this report).

Not true, mouse-region-match part has been there exactly as long
as without it.

>> I don't understand this bug report too - in fact, there is no bug,
>> the purpose of the old change was to remove gratuitous optimization
>> that breaks keyboard macros (the result of yanking should not depend
>> on differences between old and new text),
>
> Sorry, but I don't understand that. What old change? What
> optimization? What macro breakage (example)? What old and
> new text? No idea what you're saying, there.

Please read the detailed explanations on the thread in
http://lists.gnu.org/archive/html/emacs-devel/2007-06/msg01642.html

>> and nobody complained during last 10+ years.
>
> I filed this bug report 4 years ago.

What bug?  The bug was already fixed long ago.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Eli Zaretskii]

> Date: Thu, 20 Sep 2018 14:55:58 -0700 (PDT)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: 18090@debbugs.gnu.org
> 
> > I don't understand this bug report too - in fact, there is no bug,
> > the purpose of the old change was to remove gratuitous optimization
> > that breaks keyboard macros (the result of yanking should not depend
> > on differences between old and new text), 
> 
> Sorry, but I don't understand that. What old change? What
> optimization? What macro breakage (example)? What old and
> new text? No idea what you're saying, there.
> 
> > and nobody complained during last 10+ years.
> 
> I filed this bug report 4 years ago. What kinds of complaints are
> you expecting?

One again, please describe the behavior that looks buggy or incorrect
to you.  The description I found in your original bug report was
unclear.  Let's forget for a moment about the code changes and their
comments, and talk about the problem itself, which must be some
behavior in some scenario.  Please describe a recipe to reproduce that
behavior.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Drew Adams]

> You are missing the <mouse-2> part above.

I wasn't missing it. I was trying to point out that that code has nothing
to do with mouse yanking. As you say, mouse-yank commands are not
supported by `delete-selection-mode' - they do not have a `yank'
`delete-selection-mode' property value.

That code, which you say could be deleted altogether, is essential for
`delete-selection-mode' to handle `yank'. Don't you agree? That was
my point. The code in question is about commands whose property
value is `yank'.

Do you agree? If not, why do you think all of that code could be deleted?

---

Anyway, I think I understand why you added the `mouse-region-match'
condition now. But it seems to me that that condition should be used,
as Eli hinted, only when `mouse-drag-copy-region' is non-nil. (It is nil
by default now.)

Do you agree? Isn't that the (only) case where you are bothered by
the mouse selection being replaced by the previous kill-ring entry
(instead of the implicit one from the mouse selection)?

IOW, do you think this addition makes sense?

(when (and (string= (buffer-substring-no-properties
                     (point) (mark))
                    (car kill-ring))
	   mouse-drag-copy-region ; <======
           (fboundp 'mouse-region-match)
           (mouse-region-match))
  (current-kill 1))

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Juri Linkov]

>> You are missing the <mouse-2> part above.
>
> I wasn't missing it. I was trying to point out that that code has nothing
> to do with mouse yanking. As you say, mouse-yank commands are not
> supported by `delete-selection-mode' - they do not have a `yank'
> `delete-selection-mode' property value.

Shouldn't mouse yanking be supported by `delete-selection-mode',
what do you think?

> That code, which you say could be deleted altogether, is essential for
> `delete-selection-mode' to handle `yank'. Don't you agree? That was
> my point. The code in question is about commands whose property
> value is `yank'.

I agree that code that calls (delete-active-region) under the
condition (eq type 'yank) is essential to handle `yank'.
But I think that (current-kill 1) is not essential
unless it renders yank useless in some cases (yet unknown).

> Do you agree? If not, why do you think all of that code could be deleted?

Neither agree nor disagree.  Please provide a recipe to show the buggy behavior
like Eli asked you to do.

> Anyway, I think I understand why you added the `mouse-region-match'
> condition now. But it seems to me that that condition should be used,
> as Eli hinted, only when `mouse-drag-copy-region' is non-nil. (It is nil
> by default now.)
>
> Do you agree? Isn't that the (only) case where you are bothered by
> the mouse selection being replaced by the previous kill-ring entry
> (instead of the implicit one from the mouse selection)?
>
> IOW, do you think this addition makes sense?
>
> (when (and (string= (buffer-substring-no-properties
>                      (point) (mark))
>                     (car kill-ring))
> 	   mouse-drag-copy-region ; <======
>            (fboundp 'mouse-region-match)
>            (mouse-region-match))
>   (current-kill 1))

I don't know yet.  Please describe the settings and steps
that would show how to test this.

Here is a template to help you to create a bug report:

0. emacs -Q

1. M-x delete-selection-mode RET

2. Eval: (setq mouse-drag-copy-region t)

3. Drag mouse to select a word in *scratch*

4. Type `M-w' to copy selected word to the kill-ring

5. Drag mouse to select another word in *scratch*

6. Type `C-y' to yank the word from the kill-ring

I see that the previously copied word correctly replaced the selected word,
so I see no bug here.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Drew Adams]

> > As you say, mouse-yank commands are not supported by
> > `delete-selection-mode' - they do not have a `yank'
> > `delete-selection-mode' property value.

(I meant `delete-selection' property value.)

> Shouldn't mouse yanking be supported by `delete-selection-mode',
> what do you think?

Dunno. Please file a bug report, if you think so. Or
feel free to bring that into this report, if you like.

What do you want it to do? Does it do what you want if you
(put 'mouse-yank-primary 'delete-selection 'yank)?
If not, what code change do you think is needed?

> I agree that code that calls (delete-active-region) under the
> condition (eq type 'yank) is essential to handle `yank'.
> But I think that (current-kill 1) is not essential
> unless it renders yank useless in some cases (yet unknown).

Maybe you're right; dunno.

Do you want to remove that code? Without it, does
`deletion-selection-mode' do what you want for both
t and nil values of `mouse-drag-copy-region'?

At this point it's not clear to me what behavior is  bugged,
if any. I'd like to hear what changes, if any, you propose,
both for the code and the desired behavior (for both t
and nil values of `mouse-drag-copy-region').

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Juri Linkov]

>> Shouldn't mouse yanking be supported by `delete-selection-mode',
>> what do you think?
>
> Dunno. Please file a bug report, if you think so. Or
> feel free to bring that into this report, if you like.
>
> What do you want it to do? Does it do what you want if you
> (put 'mouse-yank-primary 'delete-selection 'yank)?
> If not, what code change do you think is needed?

The reason why I asked this question is because it's the only case
what might fail AFAICS.  When I tried what you suggested

(put 'mouse-yank-primary 'delete-selection 'yank)

I see that it pastes at the wrong place - before yanking
it moves point at the offset of the distance between the
region beginning and the position clicked with the mouse,
but since delete-selection-mode already deleted the region,
this means that it puts point at unrelated text that
shifts into this position after deleting the region.

mouse-yank-primary provides the correct result only when
mouse-yank-at-point is non-nil.  I don't see how this could be
supported within the limitations of the current design of
delete-selection-mode.  One way is to reimplement delete-selection-mode
using advices - it's easy to put advice around mouse-yank-at-point
that would set mouse-yank-at-point to t around the call to
mouse-yank-at-point.

Another not so good solution is to hard-code checks for
delete-selection-mode in mouse-yank-primary.

So after mouse-yank-primary is supported, we could look
under what conditions to call (current-kill 1)
in delete-selection-helper.

Please note that the current condition in (eq type 'yank)
that checks for (car kill-ring) is unsuitable
because mouse-yank-primary pastes from
(gui-get-primary-selection), not from (car kill-ring)

>> I agree that code that calls (delete-active-region) under the
>> condition (eq type 'yank) is essential to handle `yank'.
>> But I think that (current-kill 1) is not essential
>> unless it renders yank useless in some cases (yet unknown).
>
> Maybe you're right; dunno.
>
> Do you want to remove that code? Without it, does
> `deletion-selection-mode' do what you want for both
> t and nil values of `mouse-drag-copy-region'?
>
> At this point it's not clear to me what behavior is  bugged,
> if any. I'd like to hear what changes, if any, you propose,
> both for the code and the desired behavior (for both t
> and nil values of `mouse-drag-copy-region').

When mouse-drag-copy-region is non-nil, I see that
(mouse-region-match) returns non-nil, so no changes
are required in delete-selection-helper for this case.

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> One again, please describe the behavior that looks buggy or incorrect
> to you.  The description I found in your original bug report was
> unclear.  Let's forget for a moment about the code changes and their
> comments, and talk about the problem itself, which must be some
> behavior in some scenario.  Please describe a recipe to reproduce that
> behavior.

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

Reading this bug report, I'm also having difficulties understanding what
the problem is.

If this problem still exists in recent Emacs versions, do you have a
recipe to reproduce the problem you're seeing, starting from "emacs -Q"?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18090: 24.4.50; `delete-selection-helper' logic changed for non-mouse region?

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Reading this bug report, I'm also having difficulties understanding what
> the problem is.
>
> If this problem still exists in recent Emacs versions, do you have a
> recipe to reproduce the problem you're seeing, starting from "emacs -Q"?

More information was requested, but no response was given within a
month, so I'm closing this bug report.  If the problem still exists,
please respond to this email and we'll reopen the bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no