bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Stefan Kangas]

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

Attached is a patch with additional test cases for
text-property-search which checks the returned match object.  Unlike
older test cases, these also check the value of prop-match-value.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Add-tests-for-text-property-search-to-check-prop-mat.patch --]
[-- Type: application/octet-stream, Size: 2763 bytes --]

From d86382bb256bbb3808351666764cb127647016d2 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Mon, 1 Jul 2019 23:14:56 +0200
Subject: [PATCH] Add tests for text-property-search to check prop-match-value

* test/lisp/emacs-lisp/text-property-search-tests.el
(with-match-test): New macro.
(text-property-search-forward-prop-match-match-face-nil-nil)
(text-property-search-forward-prop-match-match-face-bold-t)
(text-property-search-forward-prop-match-match-face-bold-nil)
(text-property-search-backward-prop-match-match-face-nil-nil)
(text-property-search-backward-prop-match-match-face-italic-t)
(text-property-search-backward-prop-match-match-face-italic-nil):
Add test cases to also verify the value of prop-match-value.
---
 .../emacs-lisp/text-property-search-tests.el  | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/test/lisp/emacs-lisp/text-property-search-tests.el b/test/lisp/emacs-lisp/text-property-search-tests.el
index 47db54a051..21a25c7835 100644
--- a/test/lisp/emacs-lisp/text-property-search-tests.el
+++ b/test/lisp/emacs-lisp/text-property-search-tests.el
@@ -108,6 +108,49 @@ text-property-search-backward-partial-non-current-bold-t
              '("bold1")
              35))
 
+(defmacro with-match-test (form beginning end value &optional point)
+  `(with-temp-buffer
+     (text-property-setup)
+     (when ,point
+       (goto-char ,point))
+     (should (equal ,form
+                    (make-prop-match :beginning ,beginning
+                                     :end ,end
+                                     :value ,value)))))
+
+(ert-deftest text-property-search-forward-prop-match-match-face-nil-nil ()
+  (with-match-test
+   (text-property-search-forward 'face nil nil)
+   9 14 'bold))
+
+(ert-deftest text-property-search-forward-prop-match-match-face-bold-t ()
+  (with-match-test
+   (text-property-search-forward 'face 'bold t)
+   9 14 'bold))
+
+(ert-deftest text-property-search-forward-prop-match-match-face-bold-nil ()
+  (with-match-test
+   (text-property-search-forward 'face 'bold nil)
+   1 9 nil))
+
+(ert-deftest text-property-search-backward-prop-match-match-face-nil-nil ()
+  (with-match-test
+   (text-property-search-backward 'face nil nil)
+   39 46 'italic
+   (point-max)))
+
+(ert-deftest text-property-search-backward-prop-match-match-face-italic-t ()
+  (with-match-test
+   (text-property-search-backward 'face 'italic t)
+   39 46 'italic
+   (point-max)))
+
+(ert-deftest text-property-search-backward-prop-match-match-face-italic-nil ()
+  (with-match-test
+   (text-property-search-backward 'face 'italic nil)
+   46 57 nil
+   (point-max)))
+
 (provide 'text-property-search-tests)
 
 ;;; text-property-search-tests.el ends here
-- 
2.21.0

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Attached is a patch with additional test cases for
> text-property-search which checks the returned match object.  Unlike
> older test cases, these also check the value of prop-match-value.

Looks good; I've now applied them to the trunk.

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

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Juri Linkov]

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

>> Attached is a patch with additional test cases for
>> text-property-search which checks the returned match object.  Unlike
>> older test cases, these also check the value of prop-match-value.
>
> Looks good; I've now applied them to the trunk.

BTW, I tried to use text-property-search as a command interactively,
but it lacks support for reading the second argument VALUE.
Here is the patch that implements it:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: text-property-search-value.patch --]
[-- Type: text/x-diff, Size: 1008 bytes --]

diff --git a/lisp/emacs-lisp/text-property-search.el b/lisp/emacs-lisp/text-property-search.el
index 41ca07057e..5027bc406c 100644
--- a/lisp/emacs-lisp/text-property-search.el
+++ b/lisp/emacs-lisp/text-property-search.el
@@ -54,10 +54,14 @@ text-property-search-forward
 that's matching), and `prop-match-value' (the value of PROPERTY
 at the start of the region)."
   (interactive
-   (list
-    (let ((string (completing-read "Search for property: " obarray)))
-      (when (> (length string) 0)
-        (intern string obarray)))))
+   (let* ((property (completing-read "Search for property: " obarray))
+          (property (when (> (length property) 0)
+                      (intern property obarray)))
+          (value (when property
+                   (completing-read "Search for property value: " obarray)))
+          (value (when (> (length value) 0)
+                   (intern value obarray))))
+     (list property value)))
   (cond
    ;; No matches at the end of the buffer.
    ((eobp)

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> +   (let* ((property (completing-read "Search for property: " obarray))
> +          (property (when (> (length property) 0)
> +                      (intern property obarray)))
> +          (value (when property
> +                   (completing-read "Search for property value: " obarray)))
> +          (value (when (> (length value) 0)
> +                   (intern value obarray))))

Hm...  well, the value doesn't have to be a symbol, so I don't think
this is quite right -- it could be a string, or, well, anything.  I
think that's why it doesn't prompt for value in the interactive form,
because it didn't seem useful.

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

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Juri Linkov]

>> +   (let* ((property (completing-read "Search for property: " obarray))
>> +          (property (when (> (length property) 0)
>> +                      (intern property obarray)))
>> +          (value (when property
>> +                   (completing-read "Search for property value: " obarray)))
>> +          (value (when (> (length value) 0)
>> +                   (intern value obarray))))
>
> Hm...  well, the value doesn't have to be a symbol, so I don't think
> this is quite right -- it could be a string, or, well, anything.  I
> think that's why it doesn't prompt for value in the interactive form,
> because it didn't seem useful.

But searching for a certain font-lock face value is the most useful
application of this command.  How do you do that?

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> But searching for a certain font-lock face value is the most useful
> application of this command.  How do you do that?

Non-interactively?

(I've never used it to search for any faces.  :-))

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

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Juri Linkov]

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

>> But searching for a certain font-lock face value is the most useful
>> application of this command.  How do you do that?
>
> Non-interactively?
>
> (I've never used it to search for any faces.  :-))

It is useful interactively to find errors fontified
with the error face.  Here is a better patch that
can read symbols as well as strings:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: text-property-search-value.2.patch --]
[-- Type: text/x-diff, Size: 977 bytes --]

diff --git a/lisp/emacs-lisp/text-property-search.el b/lisp/emacs-lisp/text-property-search.el
index 41ca07057e..9c45cee3c1 100644
--- a/lisp/emacs-lisp/text-property-search.el
+++ b/lisp/emacs-lisp/text-property-search.el
@@ -54,10 +54,13 @@ text-property-search-forward
 that's matching), and `prop-match-value' (the value of PROPERTY
 at the start of the region)."
   (interactive
-   (list
-    (let ((string (completing-read "Search for property: " obarray)))
-      (when (> (length string) 0)
-        (intern string obarray)))))
+   (let* ((property (completing-read "Search for property: " obarray))
+          (property (when (> (length property) 0)
+                      (intern property obarray)))
+          (value (when property
+                   (read-from-minibuffer "Search for property value: "
+                                         nil nil t nil "nil"))))
+     (list property value)))
   (cond
    ;; No matches at the end of the buffer.
    ((eobp)

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> It is useful interactively to find errors fontified
> with the error face.  Here is a better patch that
> can read symbols as well as strings:

[...]

> +          (value (when property
> +                   (read-from-minibuffer "Search for property value: "
> +                                         nil nil t nil "nil"))))
> +     (list property value)))

I don't understand -- this will still return a symbol.  (And error out
if you enter stuff like "foo bar".)

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

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Juri Linkov]

>> It is useful interactively to find errors fontified
>> with the error face.  Here is a better patch that
>> can read symbols as well as strings:
>
> [...]
>
>> +          (value (when property
>> +                   (read-from-minibuffer "Search for property value: "
>> +                                         nil nil t nil "nil"))))
>> +     (list property value)))
>
> I don't understand -- this will still return a symbol.  (And error out
> if you enter stuff like "foo bar".)

Yes, entering a symbol returns a symbol, entering a string
like "foo bar" returns a string "foo bar" literally.

Currently text-property-search-forward has more problems:
today I needed to search the property ‘face’ with the value
‘hi-yellow’ in the buffer with regexps highlighted by hi-lock.el.
Executing interactively:

  M-x text-property-search-forward RET face RET hi-yellow RET

failed to find the property because all hi-lock occurrences were
combined with font-lock text properties, i.e. all they had the
property ‘face’ with the value ‘(hi-yellow font-lock-keyword-face)’
and text-property-search-forward fails to find a value in the list
of values.

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

>> I don't understand -- this will still return a symbol.  (And error out
>> if you enter stuff like "foo bar".)
>
> Yes, entering a symbol returns a symbol, entering a string
> like "foo bar" returns a string "foo bar" literally.

Oh, right.  Hm.  Well, entering things with "..." is very unusual in
Emacs prompts, so if we want that, the prompt should at least say that
that's what's expected...

> Currently text-property-search-forward has more problems:
> today I needed to search the property ‘face’ with the value
> ‘hi-yellow’ in the buffer with regexps highlighted by hi-lock.el.
> Executing interactively:
>
>   M-x text-property-search-forward RET face RET hi-yellow RET
>
> failed to find the property because all hi-lock occurrences were
> combined with font-lock text properties, i.e. all they had the
> property ‘face’ with the value ‘(hi-yellow font-lock-keyword-face)’
> and text-property-search-forward fails to find a value in the list
> of values.

Yes, you can't really use text-property-search-forward to do that in any
meaningful manner, which is why I didn't add that to the interactive
bit.  It's a function useful almost only when programming, and the only
useful interactive thing is to vaguely poke around in the buffer.

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

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Juri Linkov]

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

>>> I don't understand -- this will still return a symbol.  (And error out
>>> if you enter stuff like "foo bar".)
>>
>> Yes, entering a symbol returns a symbol, entering a string
>> like "foo bar" returns a string "foo bar" literally.
>
> Oh, right.  Hm.  Well, entering things with "..." is very unusual in
> Emacs prompts, so if we want that, the prompt should at least say that
> that's what's expected...

Fixed in the following patch.

>> Currently text-property-search-forward has more problems:
>> today I needed to search the property ‘face’ with the value
>> ‘hi-yellow’ in the buffer with regexps highlighted by hi-lock.el.
>> Executing interactively:
>>
>>   M-x text-property-search-forward RET face RET hi-yellow RET
>>
>> failed to find the property because all hi-lock occurrences were
>> combined with font-lock text properties, i.e. all they had the
>> property ‘face’ with the value ‘(hi-yellow font-lock-keyword-face)’
>> and text-property-search-forward fails to find a value in the list
>> of values.
>
> Yes, you can't really use text-property-search-forward to do that in any
> meaningful manner, which is why I didn't add that to the interactive
> bit.  It's a function useful almost only when programming, and the only
> useful interactive thing is to vaguely poke around in the buffer.

Implemented in this patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: text-property-search-value.3.patch --]
[-- Type: text/x-diff, Size: 1434 bytes --]

diff --git a/lisp/emacs-lisp/text-property-search.el b/lisp/emacs-lisp/text-property-search.el
index 41ca07057e..a44516992a 100644
--- a/lisp/emacs-lisp/text-property-search.el
+++ b/lisp/emacs-lisp/text-property-search.el
@@ -54,10 +54,13 @@ text-property-search-forward
 that's matching), and `prop-match-value' (the value of PROPERTY
 at the start of the region)."
   (interactive
-   (list
-    (let ((string (completing-read "Search for property: " obarray)))
-      (when (> (length string) 0)
-        (intern string obarray)))))
+   (let* ((property (completing-read "Search for property: " obarray))
+          (property (when (> (length property) 0)
+                      (intern property obarray)))
+          (value (when property
+                   (read-from-minibuffer "Search for property value (quote strings): "
+                                         nil nil t nil "nil"))))
+     (list property value)))
   (cond
    ;; No matches at the end of the buffer.
    ((eobp)
@@ -200,7 +203,9 @@ text-property--match-p
     (setq predicate #'equal))
    ((eq predicate nil)
     (setq predicate (lambda (val p-val)
-                      (not (equal val p-val))))))
+                      (not (if (and (listp p-val) (not (listp val)))
+                               (member val p-val)
+                             (equal val p-val)))))))
   (funcall predicate value prop-value))
 
 (provide 'text-property-search)

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> -                      (not (equal val p-val))))))
> +                      (not (if (and (listp p-val) (not (listp val)))
> +                               (member val p-val)
> +                             (equal val p-val)))))))

No, that's not an acceptable change, I think -- this function is a
search primitive, not a DWIM thing.  There's an abundance of things that
can be stored in text properties, to be compared with any number of
predicates.  You're trying to special-case it to search for faces, for
some reason, and that's not what it's for.

If you want a function to search for faces, that's something you can
write (based on this function), but that's not what this function is for.

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

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Juri Linkov]

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

>> -                      (not (equal val p-val))))))
>> +                      (not (if (and (listp p-val) (not (listp val)))
>> +                               (member val p-val)
>> +                             (equal val p-val)))))))
>
> No, that's not an acceptable change, I think -- this function is a
> search primitive, not a DWIM thing.  There's an abundance of things that
> can be stored in text properties, to be compared with any number of
> predicates.  You're trying to special-case it to search for faces, for
> some reason, and that's not what it's for.

Then it's possible to add lambda to `predicate' arg in interactive spec only
like below.

> If you want a function to search for faces, that's something you can
> write (based on this function), but that's not what this function is for.

Of course, I can write and add it to ~/.emacs.  But the question is:
do you think this is generally useful?  For example, to search
hi-lock properties, etc.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: text-property-search-value.4.patch --]
[-- Type: text/x-diff, Size: 1203 bytes --]

diff --git a/lisp/emacs-lisp/text-property-search.el b/lisp/emacs-lisp/text-property-search.el
index 41ca07057e..867094539f 100644
--- a/lisp/emacs-lisp/text-property-search.el
+++ b/lisp/emacs-lisp/text-property-search.el
@@ -54,10 +54,16 @@ text-property-search-forward
 that's matching), and `prop-match-value' (the value of PROPERTY
 at the start of the region)."
   (interactive
-   (list
-    (let ((string (completing-read "Search for property: " obarray)))
-      (when (> (length string) 0)
-        (intern string obarray)))))
+   (let* ((property (completing-read "Search for property: " obarray))
+          (property (when (> (length property) 0)
+                      (intern property obarray)))
+          (value (when property
+                   (read-from-minibuffer "Search for property value (quote strings): "
+                                         nil nil t nil "nil"))))
+     (list property value (lambda (val p-val)
+                            (not (if (and (listp p-val) (not (listp val)))
+                                     (member val p-val)
+                                   (equal val p-val)))))))
   (cond
    ;; No matches at the end of the buffer.
    ((eobp)

bug#36486: [PATCH] Add tests for text-property-search to check prop-match-value

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Of course, I can write and add it to ~/.emacs.  But the question is:
> do you think this is generally useful?  For example, to search
> hi-lock properties, etc.

I don't think this code complication belongs in that function.  If there
should be a function to search for faces, then face-search-forward
should be written (and it would be a very simple call to
text-property-search-forward with the correct predicate).

But I haven't heard anybody clamouring for such a command, really...

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