New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strange font lock of swiper #651

Closed
Yevgnen opened this Issue Sep 5, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@Yevgnen
Contributor

Yevgnen commented Sep 5, 2016

Hi!

I read the excellent post Better fuzzy matching support in Ivy and try it(integrating flx) in my Emacs, but find something strange of swiper.

Inputing exactly the same thing (the candidate as manually input content and the query) as the last image in the link, swiper and ivy-minibuffer-* gives

screen shot 2016-09-05 at 20 21 02

But counsel-M-x gives
screen shot 2016-09-05 at 20 25 01
which is the same as the image given in the post.

Shouldn't they have similar font lock ? I mean, the matched neighbouring char should have the same color in the swiper case.

A minimum test snippet is

(require 'package)

(setq package-archives '(("marmalade" . "http://marmalade-repo.org/packages/")
                         ("gnu" . "http://elpa.gnu.org/packages/")
                         ("melpa" . "https://melpa.org/packages/")
                         ("org" . "http://orgmode.org/elpa/")))

(package-initialize)

(unless (and (file-exists-p (expand-file-name "elpa/archives/marmalade" user-emacs-directory))
             (file-exists-p (expand-file-name "elpa/archives/gnu" user-emacs-directory))
             (file-exists-p (expand-file-name "elpa/archives/melpa" user-emacs-directory))
             (file-exists-p (expand-file-name "elpa/archives/org" user-emacs-directory)))
  (package-refresh-contents))

(unless (package-installed-p 'use-package)
  (package-refresh-contents)
  (package-install 'use-package))

(setq use-package-verbose t
      use-package-enable-imenu-support t)
(require 'use-package)

(use-package swiper
  :ensure t
  :bind (("C-s" . swiper))
  :init (setq ivy-height 15
              ivy-count-format "(%d/%d) "
              ivy-use-virtual-buffers t
              ivy-initial-inputs-alist nil
              ivy-re-builders-alist '((t . ivy--regex-fuzzy))
              swiper-include-line-number-in-search t)

  (use-package flx
    :ensure t
    :after swiper))

;; Test content from the post
;; visual-line-mode
;; global-visual-line-mode
;; doc-view-set-slice-using-mouse
;; vc-git-log-view-mode
;; vc-git-log-edit-mode
;; hydra-vi-toggle/whitespace-mode-and-exit
;; variable-pitch-mode
;; hydra-vi-toggle/auto-fill-mode-and-exit
;; doc-view-set-slice-from-bounding-box

Thanks!

@abo-abo abo-abo closed this in 94f3368 Sep 5, 2016

abo-abo added a commit that referenced this issue Sep 5, 2016

Improve fuzzy swiper highlight
* ivy.el (ivy--regex-fuzzy): Is non-greedy now - flx does the same.
(ivy--format-minibuffer-line): Ensure flx is available.

* ivy-test.el (ivy--regex-fuzzy): Update test.

* swiper.el (swiper--add-overlays): Join contiguous matches together.
(swiper--add-overlay): New defun.

Fixes #651
@abo-abo

This comment has been minimized.

Owner

abo-abo commented Sep 5, 2016

Please test, I had to change a lot of stuff.

@Yevgnen

This comment has been minimized.

Contributor

Yevgnen commented Sep 5, 2016

Thanks for you quick fix ! I'm glad to see this after I wake up.

Now I find something new in the newest commit
screen shot 2016-09-06 at 07 45 03

You can see that regex match in the minibuffer and in the origin buffer is inconsistent. For instance, I think both of them should highlight the substring name.

@abo-abo

This comment has been minimized.

Owner

abo-abo commented Sep 6, 2016

The highlight in the buffer is new, and I think it's superior:
swiper-fuzzy-highlight

The highlight in the minibuffer is done using flx, which is usually used for fuzzy sorting (all commands except swiper). I think it's useful to keep the highlights by flx in the minibuffer, since it gives insight into how it sorts. But removing the flx highlight specifically for swiper isn't worth the hassle, I think. But I'll keep it in mind, might replace it with the new highlight function once it's stable.

@Yevgnen

This comment has been minimized.

Contributor

Yevgnen commented Sep 6, 2016

Thanks for your explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment