Skip to content

Commit

Permalink
Fix highlighting with diacritics (#2470)
Browse files Browse the repository at this point in the history
  • Loading branch information
@thierryvolpiatto
thierryvolpiatto committed Mar 31, 2022
1 parent 53c22dc commit 16ab10d
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 15 deletions.
25 changes: 16 additions & 9 deletions helm-core.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4465,12 +4465,14 @@ useful when the order of the candidates is meaningful, e.g. with
`recentf-list'."
(helm-fuzzy-matching-default-sort-fn-1 candidates nil nil t))

(defun helm--maybe-get-migemo-pattern (pattern)
(defun helm--maybe-get-migemo-pattern (pattern &optional diacritics)
(or (and helm-migemo-mode
(assoc-default pattern helm-mm--previous-migemo-info))
pattern))
(if diacritics
(char-fold-to-regexp pattern)
pattern)))

(defun helm-fuzzy-default-highlight-match (candidate)
(defun helm-fuzzy-default-highlight-match (candidate &optional diacritics)
"The default function to highlight matches in fuzzy matching.
Highlight elements in CANDIDATE matching `helm-pattern' according
to the matching method in use."
Expand All @@ -4481,7 +4483,7 @@ to the matching method in use."
(let* ((pair (and (consp candidate) candidate))
(display (helm-stringify (if pair (car pair) candidate)))
(real (cdr pair))
(regex (helm--maybe-get-migemo-pattern helm-pattern))
(regex (helm--maybe-get-migemo-pattern helm-pattern diacritics))
(mp (pcase (get-text-property 0 'match-part display)
((pred (string= display)) nil)
(str str)))
Expand Down Expand Up @@ -4510,8 +4512,11 @@ to the matching method in use."
(when (zerop count)
(cl-loop with multi-match = (string-match-p " " helm-pattern)
with patterns = (if multi-match
(mapcar #'helm--maybe-get-migemo-pattern
(helm-mm-split-pattern helm-pattern))
(cl-loop for pat in (helm-mm-split-pattern
helm-pattern)
collect
(helm--maybe-get-migemo-pattern
pat diacritics))
(split-string helm-pattern "" t))
for p in patterns
;; Multi matches (regexps patterns).
Expand All @@ -4534,12 +4539,14 @@ to the matching method in use."
(setq display (if mp (concat beg-str (buffer-string) end-str) (buffer-string))))
(if real (cons display real) display))))

(defun helm-fuzzy-highlight-matches (candidates _source)
(defun helm-fuzzy-highlight-matches (candidates source)
"The filtered-candidate-transformer function to highlight fuzzy matches.
See `helm-fuzzy-default-highlight-match'."
(cl-assert helm-fuzzy-matching-highlight-fn nil "Wrong type argument functionp: nil")
(cl-loop for c in candidates
collect (funcall helm-fuzzy-matching-highlight-fn c)))
(cl-loop with diac = (memq 'helm-mm-3-match-on-diacritics

This comment has been minimized.

Sign in to view

Copy link
@suatkarakusoglu

suatkarakusoglu Mar 31, 2022

Hello :), I do not have extensive knowledge of emacs lisp, but at latest spacemacs distribution, it seems this function throws an exception when Spc-Spc is called.

helm-fuzzy-highlight-matches: Wrong number of arguments: (1 . 1), 2
I have manually reverted my code and it started working.

Thank you for your hard efforts 🍎

This comment has been minimized.

Sign in to view

Copy link
@thierryvolpiatto

thierryvolpiatto via email Apr 1, 2022

Author Member

This comment has been minimized.

Sign in to view

Copy link
@svenpanne

svenpanne Apr 1, 2022

Just FYI: I had a similar problem, and it effectively makes Spacemacs unusable, you get an error as soon as you hit e.g. M-x. I am not sure if it is the helm-flx or helm-descbinds package which needs some update, but reverting helm-fuzzy-highlight-matches to its previous state made things working again.

This is only an ugly workaround, perhaps the best thing would probably be making this funtion here more flexible in what it accepts. This is probably unavoidable, the other packages haven't seen an update for quite some time.

This comment has been minimized.

Sign in to view

Copy link
@thierryvolpiatto

thierryvolpiatto via email Apr 1, 2022

Author Member

This comment has been minimized.

Sign in to view

Copy link
@svenpanne

svenpanne Apr 1, 2022 •
edited
Loading

What I mean is: You did an incompatible change, which is fine in itself, but it makes Spacemacs installations unusable (see e.g. syl20bnr/spacemacs#15438). People don't update their packages within Spacemacs by hand, I wouldn't even know how to do this. The real fix would be of course that some other helm addon packages follow your changes, but as I said: Some of them didn't have an update for some time, so I don't think this will happen soon.

All I ask for is: Can you please change this function here so that it works like it did before when it receives a single argument? When the whole helm ecosystem has been updated, which will probably take some time, this workaround can be removed again. As it stands, this change here does far more damage than it actually fixes.

This comment has been minimized.

Sign in to view

Copy link
@thierryvolpiatto

thierryvolpiatto via email Apr 1, 2022

Author Member

This comment has been minimized.

Sign in to view

Copy link
@svenpanne

svenpanne Apr 1, 2022

I seriously don't understand what's the problem with adding a few lines to make a whole ecosystem happy again. I don't suggest to roll this whole change back or something along that lines, just a few lines for the sake of other packages which seem to use this function here and have not been updated yet (or will never be).

And of course this change is incompatible, there is no need discussing this. I don't have a clue if this function is officially part of a public API or not, but at least it seems to be used like it is public.

This comment has been minimized.

Sign in to view

Copy link
@nigredo-tori

nigredo-tori Apr 1, 2022 •
edited
Loading

This is not reproductible outside of spacemacs i.e. from a minimal install.

I'm out of my depth here, and I don't have time to dig into it. However, I think it might be reproducible with just helm-flx package, which provides its own highlighting function, which only accepts a single parameter.

As a bandaid, we might want to inspect each candidate function, and only supply as many arguments as it can handle in the funcall below. func-arity function can probably help with this.

This comment has been minimized.

Sign in to view

Copy link
@thierryvolpiatto

thierryvolpiatto Apr 1, 2022

Author Member

AFAIK only helm-flx is using another value for helm-fuzzy-matching-highlight-fn, one can fix it like this:

diff --git a/helm-flx.el b/helm-flx.el
index f07685c..4a05953 100644
--- a/helm-flx.el
+++ b/helm-flx.el
@@ -178,10 +178,10 @@ Return candidates prefixed with basename of `helm-input' first."
          (1+ index) (+ 2 index) '(face helm-match))))
     (buffer-string)))
 
-(defun helm-flx-fuzzy-highlight-match (candidate)
+(defun helm-flx-fuzzy-highlight-match (candidate &optional diacritics)
   (require 'flx)
   (if (string-match-p " " helm-pattern)
-      (helm-fuzzy-default-highlight-match candidate)
+      (helm-fuzzy-default-highlight-match candidate diacritics)
     (let* ((candidate (helm-flx-candidate-string candidate))
            (pair (and (consp candidate) candidate))
            (display (if pair (car pair) candidate))

This comment has been minimized.

Sign in to view

Copy link
@nigredo-tori

nigredo-tori Apr 1, 2022

I've opened a PR in helm-flx.
(helm-mklist (helm-get-attr 'match source)))
for c in candidates
collect (funcall helm-fuzzy-matching-highlight-fn c diac)))


;;; helm-flex style
Expand Down
5 changes: 5 additions & 0 deletions helm-multi-match.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,11 @@ E.g. \"bar foo baz\" will match \"barfoobaz\" or \"barbazfoo\" but not
(multi3p #'helm-mm-3p-match))))
(funcall fun candidate pattern)))

(cl-defun helm-mm-3-match-on-diacritics (candidate &optional (pattern helm-pattern))
"Same as `helm-mm-3-match' but match on diacritics if possible."
(let ((helm-mm--match-on-diacritics t))
(helm-mm-match candidate pattern)))

(defun helm-mm-search (pattern &rest _ignore)
"Search for PATTERN with `helm-mm-matching-method' function."
(let ((fun (cl-ecase helm-mm-matching-method
Expand Down
9 changes: 3 additions & 6 deletions helm-source.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -976,11 +976,6 @@ Arguments ARGS are keyword value pairs as defined in CLASS."
(defvar helm-mm-default-search-functions)
(defvar helm-mm-default-match-functions)

(defun helm-source-default-match-fns (diacritics)
(list 'helm-mm-exact-match (lambda (candidate &optional _pattern)
(let ((helm-mm--match-on-diacritics diacritics))
(helm-mm-match candidate)))))

(defun helm-source-mm-get-search-or-match-fns (source method)
(let* (diacritics
(defmatch (helm-aif (slot-value source 'match)
Expand All @@ -1003,7 +998,9 @@ Arguments ARGS are keyword value pairs as defined in CLASS."
defmatch '(helm-mm-3-migemo-match)))
(defmatch
(append helm-mm-default-match-functions defmatch))
(t (helm-source-default-match-fns diacritics))))
(t (if diacritics
(list 'helm-mm-exact-match 'helm-mm-3-match-on-diacritics)
helm-mm-default-match-functions))))
(search (cond (defsearch-strict)
(migemo
(append helm-mm-default-search-functions
Expand Down

1 comment on commit 16ab10d

@Bost
Copy link

@Bost Bost commented on 16ab10d Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even better stop using Spacemacs.

@thierryvolpiatto Spacemacs is one of the most widely used Emacs distributions if not the most one in use with 22,1K stars here on github. Just saying.

Edit:

So don't expect any change on the helm side for this.

From https://develop.spacemacs.org/layers/+completion/helm/README.html
Ivy layer is a replacement layer for Helm. When you add ivy to the existing list in your dotfile, it will completely replace the helm layer.

Please sign in to comment.