Skip to content
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

Stop completion about tempo if no matching prefix #1147

Closed
wants to merge 1 commit into from
Closed

Stop completion about tempo if no matching prefix #1147

wants to merge 1 commit into from

Conversation

condy0919
Copy link
Contributor

No description provided.

@condy0919
Copy link
Contributor Author

condy0919 commented Jul 25, 2021

Before this patch, a single " can trigger company to complete about tempo.

image

After this patch, only matched prefixes will trigger the completion. It makes senses.

image

@condy0919
Copy link
Contributor Author

I found more characters can lead to completion before this patch, e.g. >, ", . which is annoying when programming.

@dgutov
Copy link
Member

dgutov commented Jul 25, 2021

Do you only see this problem with company-tempo?

What's your value of company-minimum-prefix-length?

@condy0919
Copy link
Contributor Author

condy0919 commented Jul 25, 2021

Do you only see this problem with company-tempo?

What's your value of company-minimum-prefix-length?

3, the default.

It can be reproduced by the following company conf.

(use-package company
  :ensure t
  :hook (prog-mode . company-mode)
  :bind (:map company-mode-map
         ([remap completion-at-point] . company-complete)
         :map company-active-map
         ("C-s"     . company-filter-candidates)
         ([tab]     . company-complete-common-or-cycle)
         ([backtab] . company-select-previous-or-abort))
  :custom
  (company-idle-delay 0)
  ;; Easy navigation to candidates with M-<n>
  (company-show-quick-access t)
  (company-require-match nil)
  (company-minimum-prefix-length 3)
  (company-tooltip-width-grow-only t)
  (company-tooltip-align-annotations t)
  ;; complete `abbrev' only in current buffer and make dabbrev case-sensitive
  (company-dabbrev-other-buffers nil)
  (company-dabbrev-ignore-case nil)
  (company-dabbrev-downcase nil)
  ;; make dabbrev-code case-sensitive
  (company-dabbrev-code-ignore-case nil)
  (company-dabbrev-code-everywhere t)
  ;; call `tempo-expand-if-complete' after completion
  (company-tempo-expand t)
  ;; Stop annoying me
  (company-etags-use-main-table-list nil)
  ;; No icons
  (company-format-margin-function nil)
  (company-backends '((company-capf :with company-tempo)
                      company-files
                      (company-dabbrev-code company-keywords)
                      company-dabbrev
                      ;; HACK: prevent `lsp-mode' to add `company-capf' back.
                      company-capf)))

;; lsp-mode
(use-package lsp-mode
  :ensure t
  :hook ((lsp-mode . lsp-enable-which-key-integration)
         (prog-mode . lsp-deferred))
  :bind (:map lsp-mode-map
         ("C-c f" . lsp-format-region)
         ("C-c d" . lsp-describe-thing-at-point)
         ("C-c a" . lsp-execute-code-action)
         ("C-c r" . lsp-rename))
  :custom
  (lsp-keymap-prefix "C-c l")
  (lsp-enable-links nil)                 ;; no clickable links
  (lsp-enable-folding nil)               ;; use `hideshow' instead
  (lsp-enable-snippet nil)               ;; no snippets, it requires `yasnippet'
  (lsp-enable-file-watchers nil)         ;; performance matters
  (lsp-enable-text-document-color nil)   ;; as above
  (lsp-enable-symbol-highlighting nil)   ;; as above
  (lsp-enable-on-type-formatting nil)    ;; as above
  (lsp-enable-indentation nil)           ;; don't change my code without my permission
  (lsp-headerline-breadcrumb-enable nil) ;; keep headline clean
  (lsp-modeline-code-actions-enable nil) ;; keep modeline clean
  (lsp-modeline-diagnostics-enable nil)  ;; as above
  (lsp-log-io nil)                       ;; debug only
  (lsp-auto-guess-root t)                ;; auto guess root
  (lsp-keep-workspace-alive nil)         ;; auto kill lsp server
  (lsp-eldoc-enable-hover nil))          ;; disable eldoc hover

(use-package tempo
  :ensure nil
  :after cc-mode
  :hook ((c-mode . c-mode-tempo-setup)
         (c++-mode . c++-mode-tempo-setup))
  :config
  (defvar c-tempo-tags nil)
  (defvar c++-tempo-tags nil)

  (defun c-mode-tempo-setup ()
    (tempo-use-tag-list 'c-tempo-tags))
  (defun c++-mode-tempo-setup ()
    (tempo-use-tag-list 'c-tempo-tags)
    (tempo-use-tag-list 'c++-tempo-tags))

  (tempo-define-template "c-ifndef"
                         '("#ifndef " (P "ifndef-clause: " clause) > n
                           "#define " (s clause) n> p n
                           "#endif // " (s clause) n>
                           )
                         "ifndef"
                         "Insert #ifndef #define #endif directives"
                         'c-tempo-tags)
  (tempo-define-template "c-main"
                         '("int main(int argc, char* argv[]) {" > n>
                           p n
                           "}" > n>
                           )
                         "main"
                         "Insert a main function"
                         'c-tempo-tags)

  (tempo-define-template "c++-class-equality"
                         '((P "type: " type 'noinsert)
                           (tempo-save-named 'inline (if (y-or-n-p "Inline? ") "inline " ""))
                           (tempo-save-named 'constexpr (if (y-or-n-p "Constexpr? ") "constexpr " ""))
                           (tempo-save-named 'noexcept (if (y-or-n-p "Noexcept? ") "noexcept " ""))
                           (s inline) (s constexpr) "bool operator==(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                           p n
                           "}" > n>
                           (s inline) (s constexpr) "bool operator!=(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                           "return !(lhs == rhs);" > n>
                           "}" > n>
                           )
                         "eq"
                         "Class equality comparison"
                         'c++-tempo-tags)
  (tempo-define-template "c++-class-totally-ordered"
                         '((P "type: " type 'noinsert)
                           (tempo-save-named 'inline (if (y-or-n-p "Inline? ") "inline " ""))
                           (tempo-save-named 'constexpr (if (y-or-n-p "Constexpr? ") "constexpr " ""))
                           (tempo-save-named 'noexcept (if (y-or-n-p "Noexcept? ") "noexcept " ""))
                           (s inline) (s constexpr) "bool operator<(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                           p n
                           "}" > n>
                           (s inline) (s constexpr) "bool operator>(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                           "return rhs < lhs;" > n>
                           "}" > n>
                           (s inline) (s constexpr) "bool operator<=(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                           "return !(lhs > rhs);" > n>
                           "}" > n>
                           (s inline) (s constexpr) "bool operator>=(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                           "return !(lhs < rhs);" > n>
                           "}" > n>
                           )
                         "ord"
                         "Class totally ordered comparison"
                         'c++-tempo-tags))

Open a cpp file, then type " to see the result.

image

@dgutov
Copy link
Member

dgutov commented Jul 25, 2021

3, the default

Did you check the value at runtime, or did you just look at the config for the answer?

I don't have use-package installed, and I don't have any C++ language servers at hand. Can you provide a repro without use-package and lsp-mode? company-clang could be the replacement for company-capf in that scenario.

I tried reproducing this problem by copying just the necessary parts of your config and failed. The templates were working, but no trace of the problem from the description.

@condy0919
Copy link
Contributor Author

condy0919 commented Jul 25, 2021

3, the default

Did you check the value at runtime, or did you just look at the config for the answer?

Both are 3.

I don't have use-package installed, and I don't have any C++ language servers at hand. Can you provide a repro without use-package and lsp-mode? company-clang could be the replacement for company-capf in that scenario.

I tried reproducing this problem by copying just the necessary parts of your config and failed. The templates were working, but no trace of the problem from the description.

After using company-clang, it can be only reproduced after "., see screenshots below.

(require 'tempo)
(require 'cc-mode)

(setq company-backends '((company-clang :with company-tempo)
                         company-files
                         (company-dabbrev-code company-keywords)
                         company-dabbrev))
(setq company-idle-delay 0)
(setq company-require-match nil)
(setq company-minimum-prefix-length 3)
(setq company-tempo-expand t)



(defvar c-tempo-tags nil)
(defvar c++-tempo-tags nil)

(defun c-mode-tempo-setup ()
  (tempo-use-tag-list 'c-tempo-tags))
(defun c++-mode-tempo-setup ()
  (tempo-use-tag-list 'c-tempo-tags)
  (tempo-use-tag-list 'c++-tempo-tags))

(tempo-define-template "c-ifndef"
                       '("#ifndef " (P "ifndef-clause: " clause) > n
                         "#define " (s clause) n> p n
                         "#endif // " (s clause) n>
                         )
                       "ifndef"
                       "Insert #ifndef #define #endif directives"
                       'c-tempo-tags)
(tempo-define-template "c-main"
                       '("int main(int argc, char* argv[]) {" > n>
                         p n
                         "}" > n>
                         )
                       "main"
                       "Insert a main function"
                       'c-tempo-tags)

(tempo-define-template "c++-class-equality"
                       '((P "type: " type 'noinsert)
                         (tempo-save-named 'inline (if (y-or-n-p "Inline? ") "inline " ""))
                         (tempo-save-named 'constexpr (if (y-or-n-p "Constexpr? ") "constexpr " ""))
                         (tempo-save-named 'noexcept (if (y-or-n-p "Noexcept? ") "noexcept " ""))
                         (s inline) (s constexpr) "bool operator==(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                         p n
                         "}" > n>
                         (s inline) (s constexpr) "bool operator!=(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                         "return !(lhs == rhs);" > n>
                         "}" > n>
                         )
                       "eq"
                       "Class equality comparison"
                       'c++-tempo-tags)
(tempo-define-template "c++-class-totally-ordered"
                       '((P "type: " type 'noinsert)
                         (tempo-save-named 'inline (if (y-or-n-p "Inline? ") "inline " ""))
                         (tempo-save-named 'constexpr (if (y-or-n-p "Constexpr? ") "constexpr " ""))
                         (tempo-save-named 'noexcept (if (y-or-n-p "Noexcept? ") "noexcept " ""))
                         (s inline) (s constexpr) "bool operator<(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                         p n
                         "}" > n>
                         (s inline) (s constexpr) "bool operator>(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                         "return rhs < lhs;" > n>
                         "}" > n>
                         (s inline) (s constexpr) "bool operator<=(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                         "return !(lhs > rhs);" > n>
                         "}" > n>
                         (s inline) (s constexpr) "bool operator>=(const " (s type) "& lhs, const " (s type) "& rhs) " (s noexcept) "{" > n>
                         "return !(lhs < rhs);" > n>
                         "}" > n>
                         )
                       "ord"
                       "Class totally ordered comparison"
                       'c++-tempo-tags)

(add-hook 'c++-mode-hook #'c++-mode-tempo-setup)

Open a cpp file, type ". to see the result.

image

More cases

image

@dgutov
Copy link
Member

dgutov commented Jul 25, 2021

After using company-clang, it can be only reproduced after "., see screenshots below.

So basically after any ., right? Or :: or ->, I suppose.

If so, I think I understand what's going on.

@condy0919
Copy link
Contributor Author

After using company-clang, it can be only reproduced after "., see screenshots below.

So basically after any ., right? Or :: or ->, I suppose.

If so, I think I understand what's going on.

Yes, you're right. All :: and -> have the same issue.

@dgutov
Copy link
Member

dgutov commented Jul 26, 2021

The problem:

We have a mechanism that "overrides" the actual prefix length for the returned prefix string, when we want, in certain contexts, to initiate idle completion right away. Then that backend returns a cons ("prefix-string" . t) from the prefix handler.

company-clang does that after member access; lsp-mode does it in a more generic fashion -- after any character that the language server has designated as a trigger character, and there's usually more of the (that must make the effect worse).

Grouped backends work behind an adapter that combines them together to make them look like a single backend. But there is no good way to combine such hints (or at least none have been implemented so far) that would retain the difference between the backends WRT triggers depending on the context. What it does is simple: if the first backend says "go", completions are collected from all of them.

Not sure what's a good approach to improve this. I suppose the multi-backend adapter in its candidates implementation could track whether the current completion is done by explicitly invoking a command, poll each backend's prefix again, compare against company-minimum-prefix-length, and depending on that, skip the backend's completions. That seems a little involved, though, and might not work with third-party frontends that use company's backends. But it's something to try.

Your particular patch here seems like it will break M-x company-tempo, and it would only help in the "empty prefix" case, but not, say, when the prefix is 1 character long and the company-minimum-prefix-length has its default value of 3.

dgutov added a commit that referenced this pull request Oct 4, 2023
…eparately

This should help with
#1398,
#641,
#553,
#485,
#1147,
#840

With some of the above already closed, and some describing more serious
scenarios than others.

Anyway, this is a halfway step to dropping framework-level caching (or moving it to
backends).  For now this seems like an adequate solution, given that the most
"heavy" backend these days is `company-capf`, which disables caching already.
Though `company-dabbrev-code` is a close next.
@condy0919 condy0919 closed this by deleting the head repository Oct 6, 2023
dgutov added a commit that referenced this pull request Oct 9, 2023
…eparately (#1405)

* company--multi-backend-adapter: Consider min prefix in 'candidates' separately

This should help with
#1398,
#641,
#553,
#485,
#1147,
#840

With some of the above already closed, and some describing more serious
scenarios than others.

Anyway, this is a halfway step to dropping framework-level caching (or moving it to
backends).  For now this seems like an adequate solution (resetting the cache
sometimes when the new chars are typed), given that the most "heavy" backend
these days is `company-capf`, which disables caching already.
Though `company-dabbrev-code` is a close next.

Uses new semi-private backend action called 'set-min-prefix', to keep the merging function's backward compatibility, for the sake of external callers.
@dgutov
Copy link
Member

dgutov commented Oct 9, 2023

Turns out this could be managed inside the framework.

Check out the latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants