bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Dima Kogan]

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

Hi.

Attached is a small feature patch. Currently when a hi-lock.el function
is called interactively, the default offered by (read-regexp) is the
last value.

With the patch, the behavior is the same if no active region is
available; if an active region IS available, the default is the contents
of the region.

Perhaps something like this should be added to (read-regexp), but that's
a bigger proposal than this small patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-hi-lock-interactive-functions-now-default-to-the-act.patch --]
[-- Type: text/x-diff, Size: 1974 bytes --]

From ccf892b48f8bb93c6352f611b4256cb401222668 Mon Sep 17 00:00:00 2001
From: Dima Kogan <dima@secretsauce.net>
Date: Sun, 12 Oct 2014 15:32:02 -0700
Subject: [PATCH] hi-lock interactive functions now default to the active
 region, if there is one

---
 lisp/hi-lock.el | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/lisp/hi-lock.el b/lisp/hi-lock.el
index 98a26dd..6b06d08 100644
--- a/lisp/hi-lock.el
+++ b/lisp/hi-lock.el
@@ -419,7 +419,10 @@ highlighting will not update as you type."
   (interactive
    (list
     (hi-lock-regexp-okay
-     (read-regexp "Regexp to highlight line" 'regexp-history-last))
+     (read-regexp "Regexp to highlight line"
+                  (if mark-active
+                      (buffer-substring-no-properties (point) (mark))
+                    'regexp-history-last)))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-yellow))
   (unless hi-lock-mode (hi-lock-mode 1))
@@ -443,7 +446,10 @@ highlighting will not update as you type."
   (interactive
    (list
     (hi-lock-regexp-okay
-     (read-regexp "Regexp to highlight" 'regexp-history-last))
+     (read-regexp "Regexp to highlight"
+                  (if mark-active
+                      (buffer-substring-no-properties (point) (mark))
+                    'regexp-history-last)))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-yellow))
   (unless hi-lock-mode (hi-lock-mode 1))
@@ -468,7 +474,10 @@ highlighting will not update as you type."
    (list
     (hi-lock-regexp-okay
      (hi-lock-process-phrase
-      (read-regexp "Phrase to highlight" 'regexp-history-last)))
+      (read-regexp "Phrase to highlight"
+                   (if mark-active
+                       (buffer-substring-no-properties (point) (mark))
+                     'regexp-history-last))))
     (hi-lock-read-face-name)))
   (or (facep face) (setq face 'hi-yellow))
   (unless hi-lock-mode (hi-lock-mode 1))
-- 
2.0.0

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Drew Adams]

> the behavior is the same if no active region is
> available; if an active region IS available, the default is the
> contents of the region.
> 
> Perhaps something like this should be added to (read-regexp),
> but that's a bigger proposal than this small patch.

I don't care what you do to hi-lock. However, wrt `read-regexp':
I certainly hope not. That would make that function useless for
any command that wants to act on the active region (e.g., vs
the buffer if inactive).

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Ivan Shmakov]

>>>>> Dima Kogan <dima@secretsauce.net> writes:

 > Attached is a small feature patch. Currently when a hi-lock.el
 > function is called interactively, the default offered by
 > (read-regexp) is the last value.

 > With the patch, the behavior is the same if no active region is
 > available; if an active region IS available, the default is the
 > contents of the region.

[…]

 > --- a/lisp/hi-lock.el
 > +++ b/lisp/hi-lock.el
 > @@ -419,7 +419,10 @@ highlighting will not update as you type."
 >    (interactive
 >     (list
 >      (hi-lock-regexp-okay
 > -     (read-regexp "Regexp to highlight line" 'regexp-history-last))
 > +     (read-regexp "Regexp to highlight line"
 > +                  (if mark-active
 > +                      (buffer-substring-no-properties (point) (mark))
 > +                    'regexp-history-last)))
 >      (hi-lock-read-face-name)))
 >    (or (facep face) (setq face 'hi-yellow))
 >    (unless hi-lock-mode (hi-lock-mode 1))

	I don’t seem to understand; is mark-active ever supposed to
	become nil when /not/ in the transient-mark-mode?

	Personally, I find the latter distracting enough not to ever
	turn it on.  So, I guess that the above would mean that I’d
	always be getting anything I have for the region (which may
	easily be a substantial portion of the buffer) for the default.

	Thus, at the very least, the above should probably use
	(and transient-mark-mode mark-active) as the condition, or
	something like that.

[…]

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Stefan Monnier]

>> +                  (if mark-active

Thanks for catching it, Ivan.
Indeed testing `mark-active' (or transient-mark-mode) is basically never
a good idea.  Please use `use-region-p' instead.


        Stefan

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Juri Linkov]

>> Perhaps something like this should be added to (read-regexp),
>> but that's a bigger proposal than this small patch.
>
> I don't care what you do to hi-lock. However, wrt `read-regexp':
> I certainly hope not. That would make that function useless for
> any command that wants to act on the active region (e.g., vs
> the buffer if inactive).

There is no problem in using the active region in `read-regexp' as
we discussed it in http://thread.gmane.org/gmane.emacs.devel/168564

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Drew Adams]

> > I don't care what you do to hi-lock.  However, wrt `read-regexp':
> > I certainly hope not.  That would make that function useless for
> > any command that wants to act on the active region (e.g., vs
> > the buffer if inactive).
> 
> There is no problem in using the active region in `read-regexp' as
> we discussed it in http://thread.gmane.org/gmane.emacs.devel/168564

Huh? There is NO discussion of `read-regexp' in that thread. Zero.

And that thread certainly does not come to the conclusion that
the active region should always be taken as the default value
(of whatever).

You yourself acknowledge in that thread that using the active
region to limit the scope of a command is sometimes more useful
than using it to come up with a default string value.

`read-regexp' is not a command.  It is a utility function, which
can be used in all kinds of commands, all kinds of contexts.
We should not fix its default value in a way that limits the
use of `read-regexp' by some command/context of the active region
for something else.

What IS pointed out in that thread is that there are other ways
to pick up text at point, besides using the active region.  And
that gives you much more flexibility than hard-wiring the region
content as the default value.

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Juri Linkov]

> What IS pointed out in that thread is that there are other ways
> to pick up text at point, besides using the active region.  And
> that gives you much more flexibility than hard-wiring the region
> content as the default value.

Not *THE* default value, but a list of several default values
where the active region is its second default value
(coming after the symbol at point in that list).

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Drew Adams]

> > What IS pointed out in that thread is that there are other ways
> > to pick up text at point, besides using the active region.  And
> > that gives you much more flexibility than hard-wiring the region
> > content as the default value.
> 
> Not *THE* default value, but a list of several default values
> where the active region is its second default value
> (coming after the symbol at point in that list).

Ah, OK, if that's what you meant.  Yes, a user should be
able to choose that, by cycling - at least in some contexts
(see below).  No, in general it should not be the _default_
default value. ;-)  That is, it should not be the first in
the list.

Even so, it would be good for a command for which the active
region defines the command scope to be able to easily exclude
that value from the defaults list, as it makes no sense in
that context.

IOW, my argument still holds: It is not always the case that
it makes sense for a default value for `read-regexp' to be
the content of the active region.  Sometimes it makes a lot
of sense.  Sometimes it makes _no_ sense.  And sometimes it
can make a little sense - a nice-to-have-available choice.

What `read-regexp' is used for depends on the context.
That context (e.g. a particular command) determines whether
and how much it makes sense for the active region to be
used as a default value.

Again, `read-regexp' is a utility function.  Its use cases
are many and varied.

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Drew Adams]

Also, discussion about changing `read-regexp' should be in a different bug thread or emacs-devel discussion.  It is not especially related to "hi-lock interactive functions" defaulting.  It's not right to entertain patches to `read-regexp' in this limited context.

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Lars Ingebrigtsen]

Dima Kogan <dima@secretsauce.net> writes:

> Attached is a small feature patch. Currently when a hi-lock.el function
> is called interactively, the default offered by (read-regexp) is the
> last value.
>
> With the patch, the behavior is the same if no active region is
> available; if an active region IS available, the default is the contents
> of the region.

Hm...  I think this sounds kinda unusual?  We don't have any other
prompting functions that default to using the contents of the region (if
it's active), I think?

So I'm closing this bug report.

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

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Juri Linkov]

>> Attached is a small feature patch. Currently when a hi-lock.el function
>> is called interactively, the default offered by (read-regexp) is the
>> last value.
>>
>> With the patch, the behavior is the same if no active region is
>> available; if an active region IS available, the default is the contents
>> of the region.
>
> Hm...  I think this sounds kinda unusual?  We don't have any other
> prompting functions that default to using the contents of the region (if
> it's active), I think?

Wouldn't the following patch satisfy the request by allowing to use
M-n M-n to fetch the active region in the hi-lock command prompt:

diff --git a/lisp/replace.el b/lisp/replace.el
index 9d1b7bf747..15309a6a22 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -775,6 +775,8 @@ read-regexp-suggestions
 appends the list returned by this function to the end of values available
 via \\<minibuffer-local-map>\\[next-history-element]."
   (list
+   (when (use-region-p)
+     (buffer-substring-no-properties (region-beginning) (region-end)))
    (find-tag-default-as-regexp)
    (find-tag-default-as-symbol-regexp)
    (car regexp-search-ring)

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>> Hm...  I think this sounds kinda unusual?  We don't have any other
>> prompting functions that default to using the contents of the region (if
>> it's active), I think?
>
> Wouldn't the following patch satisfy the request by allowing to use
> M-n M-n to fetch the active region in the hi-lock command prompt:

[...]

>  appends the list returned by this function to the end of values available
>  via \\<minibuffer-local-map>\\[next-history-element]."
>    (list
> +   (when (use-region-p)
> +     (buffer-substring-no-properties (region-beginning) (region-end)))
>     (find-tag-default-as-regexp)
>     (find-tag-default-as-symbol-regexp)
>     (car regexp-search-ring)

That's still a rather unusual way for a command to work, I think.

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

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Juri Linkov]

>>> Hm...  I think this sounds kinda unusual?  We don't have any other
>>> prompting functions that default to using the contents of the region (if
>>> it's active), I think?
>>
>> Wouldn't the following patch satisfy the request by allowing to use
>> M-n M-n to fetch the active region in the hi-lock command prompt:
>
> [...]
>
>>  appends the list returned by this function to the end of values available
>>  via \\<minibuffer-local-map>\\[next-history-element]."
>>    (list
>> +   (when (use-region-p)
>> +     (buffer-substring-no-properties (region-beginning) (region-end)))
>>     (find-tag-default-as-regexp)
>>     (find-tag-default-as-symbol-regexp)
>>     (car regexp-search-ring)
>
> That's still a rather unusual way for a command to work, I think.

Then maybe to add a new command to pull the contents of the active region
into the minibuffer.

bug#18697: 25.0.50; [PATCH] hi-lock interactive functions now default to the active region, if there is one

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Then maybe to add a new command to pull the contents of the active region
> into the minibuffer.

That sounds like a good idea to me.

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