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

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
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

0001-Add-tests-for-text-property-search-to-check-prop-mat.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Stefan Kangas <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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:


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)
Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Juri Linkov <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> +   (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?



Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Juri Linkov <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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:


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)
Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Juri Linkov <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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.



Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Juri Linkov <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>>> 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:


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)
Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Juri Linkov <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> -                      (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.


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)
Reply | Threaded
Open this post in threaded view
|

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

Lars Ingebrigtsen
Juri Linkov <[hidden email]> 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