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

What do package writers need to do to support this? #28

Closed
cpitclaudel opened this issue Dec 31, 2015 · 26 comments
Closed

What do package writers need to do to support this? #28

cpitclaudel opened this issue Dec 31, 2015 · 26 comments
Labels

Comments

@cpitclaudel
Copy link
Collaborator

This package looks cool, so I'd love to have support for it in company-coq. Unfortunately, it doesn't seem to work out of the box: using M-h yields:

Error running timer ‘company-quickhelp--show’: (wrong-type-argument stringp (#<buffer *company-coq: documentation*> . 34))

Hovering on candidates just shows the documentation buffer (company-coq displays the window by itself, so that's expected, but no pos-tip window is shown).

@expez
Copy link
Collaborator

expez commented Jan 1, 2016

I hook into company's documentation support, so if C-h, or f1, works when a company candidate is active, then company-quickhelp should work too.

@expez expez added the question label Jan 1, 2016
@cpitclaudel
Copy link
Collaborator Author

C-h and f1 work fine :) Could it be that company-quickhelp hasn't been updated? It seems that it doesn't work when backends return a cons of a buffer and a window start position.

‘doc-buffer’: The second argument is a completion candidate.  Return a
buffer with documentation for it.  Preferably use ‘company-doc-buffer’.  If
not all buffer contents pertain to this candidate, return a cons of buffer
and window start position.

@expez
Copy link
Collaborator

expez commented Jan 1, 2016

If the company API has changed recently then that might be the case. I haven't made any changes to this package in a good while.

@cpitclaudel
Copy link
Collaborator Author

It did :) Thanks for your explanation. The changes required should be pretty minimal IIUC (instead of taking the text of the whole buffer, you would only take the text starting from a given offset)

@expez
Copy link
Collaborator

expez commented Jan 15, 2016

What do you want me to do here? Just fill the popup with the text starting from that buffer location? It seems that might give you quite a bit of irrelevant text.

AFAIK company-coq is the only package relying on alternative behavior. Perhaps you want to test this yourself and submit a PR? I suspect you've already hacked something together locally to fix this issue, as I've been so slow to address it and it's only a matter of changing a few lines of code.

@cpitclaudel
Copy link
Collaborator Author

Filling the popup from that buffer location and truncating if it gets too long sounds reasonable :) I don't think I'll have time to prepare a PR for now, but I'll keep it in mind; I haven't been looking at this issue further.

@expez expez closed this as completed in 5a3f237 Jan 28, 2016
@expez
Copy link
Collaborator

expez commented Jan 28, 2016

It would be cool if you found the time to test this @cpitclaudel.

When that's done I'll push a new tag for melpa-stable.

@cpitclaudel
Copy link
Collaborator Author

It almost works; the doc buffer is still shown for every candidate (which is not surprising: company-coq explicitly displays it when 'doc-buffer is called).

Is there a way for me to detect that the call to 'doc-buffer comes from company-quickhelp, and not display the buffer? The reason I need to display it is that otherwise shr (the html rendering engine) doesn't use the right window width.

Another issue that I'm seeing is that text properties (in particular composition) are not preserved. Should this be another bug report, or is it expected?

@expez
Copy link
Collaborator

expez commented Feb 3, 2016

Is there a way for me to detect that the call to 'doc-buffer comes from company-quickhelp

Not at the moment. You're not really adhering to the API if a request for the doc-buffer has unwanted side-effects.

If you can't think of a way to rewrite that part of the backend I'll add a dynamic var you can check for.

Another issue that I'm seeing is that text properties (in particular composition) are not preserved. Should this be another bug report, or is it expected?

This is to be expected. It's just an instance of defensive coding. I'm using buffer-substring-no-properties to get the stuff out of the doc buffer because I've no idea what happens when pos-tip is asked to render various text properties. If you want to check that out, just change buffer-substring-no-properties to buffer-substring.

@cpitclaudel
Copy link
Collaborator Author

Not at the moment. You're not really adhering to the API if a request for the doc-buffer has unwanted side-effects.

I don't think the API defines which side-effects are unwanted :) I could try to find a workaround.

This is to be expected. It's just an instance of defensive coding. I'm using buffer-substring-no-properties to get the stuff out of the doc buffer because I've no idea what happens when pos-tip is asked to render various text properties. If you want to check that out, just change buffer-substring-no-properties to buffer-substring.

I'll try it out.

@cpitclaudel
Copy link
Collaborator Author

Unrelated: the loop in company-quickhelp--doc-and-meta has quadratic complexity (line-number-at-pos takes linear time)

@cpitclaudel
Copy link
Collaborator Author

Ah, never mind, things are still broken actually:

  car(19)
  (progn (car (cdr doc)))
  (if (consp doc) (progn (car (cdr doc))))
  (let ((doc-buffer (if (consp doc) (car doc) doc)) (doc-begin (if (consp doc) (progn (car (cdr doc)))))) (save-current-buffer (set-buffer doc-buffer) (let ((truncated t)) (goto-char (or doc-begin (point-min))) (if company-quickhelp-max-lines (forward-line company-quickhelp-max-lines) (goto-char (point-max))) (beginning-of-line) (if (= (line-number-at-pos) (save-excursion (goto-char (point-max)) (line-number-at-pos))) (progn (setq truncated nil))) (while (and (not (= (line-number-at-pos) 1)) (or (looking-at-p "\\[back\\]") (looking-at-p "\\[source\\]") (looking-at-p "^\\s-*$"))) (forward-line -1)) (list :doc (buffer-substring (point-min) (point-at-eol)) :truncated truncated))))
  company-quickhelp--doc-and-meta((#<buffer *company-coq: documentation*> . 19))
  (progn (company-quickhelp--doc-and-meta doc))
  (if doc (progn (company-quickhelp--doc-and-meta doc)))
  (let* ((doc (company-call-backend (quote doc-buffer) selected)) (doc-and-meta (if doc (progn (company-quickhelp--doc-and-meta doc)))) (truncated (plist-get doc-and-meta :truncated)) (doc (plist-get doc-and-meta :doc))) (if (string= doc "") nil (if truncated (concat doc "\n\n[...]") doc)))
  (progn (fset (quote completing-read) (function company-quickhelp--completing-read)) (let* ((doc (company-call-backend (quote doc-buffer) selected)) (doc-and-meta (if doc (progn (company-quickhelp--doc-and-meta doc)))) (truncated (plist-get doc-and-meta :truncated)) (doc (plist-get doc-and-meta :doc))) (if (string= doc "") nil (if truncated (concat doc "\n\n[...]") doc))))
  (unwind-protect (progn (fset (quote completing-read) (function company-quickhelp--completing-read)) (let* ((doc (company-call-backend (quote doc-buffer) selected)) (doc-and-meta (if doc (progn (company-quickhelp--doc-and-meta doc)))) (truncated (plist-get doc-and-meta :truncated)) (doc (plist-get doc-and-meta :doc))) (if (string= doc "") nil (if truncated (concat doc "\n\n[...]") doc)))) (fset (quote completing-read) old))
  (let* ((old (symbol-function (quote completing-read)))) (unwind-protect (progn (fset (quote completing-read) (function company-quickhelp--completing-read)) (let* ((doc (company-call-backend (quote doc-buffer) selected)) (doc-and-meta (if doc (progn ...))) (truncated (plist-get doc-and-meta :truncated)) (doc (plist-get doc-and-meta :doc))) (if (string= doc "") nil (if truncated (concat doc "\n\n[...]") doc)))) (fset (quote completing-read) old)))
  company-quickhelp--doc(#("apply term" 0 6 (match-end 5 match-beginning 0 source man anchor ("6s" . 386) insert "apply @{term}" num-holes 1 company-coq-original-backend company-coq-refman-tactic-abbrevs-backend) 6 10 (match-end 5 match-beginning 0 source man anchor ("6s" . 386) insert "apply @{term}" face (company-coq-snippet-hole-face) num-holes 1 company-coq-original-backend company-coq-refman-tactic-abbrevs-backend)))
  (let* ((selected (nth company-selection company-candidates)) (doc (company-quickhelp--doc selected)) (ovl company-pseudo-tooltip-overlay) (overlay-width (* (frame-char-width) (if ovl (overlay-get ovl (quote company-width)) 0))) (overlay-position (* (frame-char-width) (- (if ovl (overlay-get ovl (quote company-column)) 1) 1))) (x-gtk-use-system-tooltips nil)) (if (and ovl doc) (progn (with-no-warnings (pos-tip-show doc nil (overlay-start ovl) nil 300 80 nil (+ overlay-width overlay-position) 1)))))
  company-quickhelp--show()
  apply(company-quickhelp--show nil)
  timer-event-handler([t 0 0 0 nil company-quickhelp--show nil idle 0])

@cpitclaudel
Copy link
Collaborator Author

Random thought: what would you think of a 'doc-string backend property? It could be standardized on the company side, and it would have much cleaner semantics. Of course doc-buffer would be used as the fallback in most cases.

@cpitclaudel
Copy link
Collaborator Author

Regarding the fontification: pos-tip-show-no-propertize keeps faces; pos-tip-show doesn't.

@expez
Copy link
Collaborator

expez commented Feb 4, 2016

Unrelated: the loop in company-quickhelp--doc-and-meta has quadratic complexity (line-number-at-pos takes linear time)

Is this a problem for you? For most usage n is less than 20 so the asymptotic runtime complexity didn't really worry me :p

Ah, never mind, things are still broken actually

I've solved this. I forgot that you pull stuff out of (1 . 2) and (1 2) differently. I mostly program in clojure these days and it doesn't have dotted lists.

Random thought: what would you think of a 'doc-string backend property?

This would be cool. \cc @dgutov

@cpitclaudel
Copy link
Collaborator Author

Thanks for the fix. And complexity doesn't seem to be too much of a problem :)
I'd like the doc-string thing to happen; it would make my life much much simpler.

@dgutov
Copy link
Member

dgutov commented Feb 4, 2016

doc-string

Why? Just call (company-doc-buffer doc-string).

@cpitclaudel
Copy link
Collaborator Author

@dgutov I don't understand your answer :/

@dgutov
Copy link
Member

dgutov commented Feb 4, 2016

doc-buffer and doc-string are basically equivalent. If we have one, why would we have the other?

You can convert from a string to a buffer, and back, pretty easily. company-doc-buffer is the suggested function to convert a string to a buffer.

@cpitclaudel
Copy link
Collaborator Author

Ah, I understand now. Thanks.

What I'm proposing is a middle ground between 'meta and 'doc-buffer. 'meta is very short and very plain. 'doc-buffer can be arbitrarily very elaborate.

The proposed change wouldn't require updating existing backends; 'doc-buffer would be used instead of doc-string if the wasn't available. But 'doc-string would be a way to give bacends information about the context in which the documentation will be displayed.

In the concrete case of company-coq, doc-buffer returns a buffer constructed by rendering an HTML document. I add a title, I highlight some parts of the document to make them stand out, I apply syntax highlighting to other parts, I enable prettify-symbols-mode, etc.

doc-string, if it existed, would do a much less invasive version of the same thing: less fontification (bold and prettify-symbols-mode, , but no larger faces and no overlays).

More practically: due to the setting in which company-coq is used, I need to force the buffer to display in a specific window; thus doc-buffer aggressively displays the buffer. Calling doc-string wouldn't cause anything to be displayed.

@dgutov
Copy link
Member

dgutov commented Feb 5, 2016

doc-string, if it existed, would do a much less invasive version of the same thing: less fontification (bold and prettify-symbols-mode, , but no larger faces and no overlays).

Why?

More practically: due to the setting in which company-coq is used, I need to force the buffer to display in a specific window; thus doc-buffer aggressively displays the buffer.

So, do you actually want doc-string to be used when the user calls company-show-doc-buffer, or not? If yes, you can return the "less invasive" output in doc-buffer. If no, that's of no concern for company-mode, is it? You can just as well define a new action to be used in company-quickhelp only, like quickhelp-string.

@cpitclaudel
Copy link
Collaborator Author

More practically: due to the setting in which company-coq is used, I need to force the buffer to display in a specific window; thus doc-buffer aggressively displays the buffer.

So, do you actually want doc-string to be used when the user calls company-show-doc-buffer, or not? If yes, you can return the "less invasive" output in doc-buffer.

No; I'm thinking of two different and independent backend commands, with the expectation that one could in most cases be implemented in terms of the other.

If no, that's of no concern for company-mode, is it? You can just as well define a new action to be used in company-quickhelp only, like quickhelp-string.

This is definitely true. But your input on these matters is both valuable and welcome :) Your efforts on company are responsible for large gains in popularity and adoption, so it seems reasonable to ask you for insight about this. Maybe I've missed something, or maybe there's a different approach that you could suggest, or...

But yeah, this isn't about changing company; just about getting your opinion on something that uses company.

@dgutov
Copy link
Member

dgutov commented Feb 5, 2016

In that case, I think quickhelp-string is the best option at the moment (at least until we find a use for both of these fairly similar actions, in company-mode proper). company-quickhelp will then use it, and fail over to doc-buffer if it's not defined.

Maybe I've missed something, or maybe there's a different approach that you could suggest, or...

Do you see any problems with the above?

@cpitclaudel
Copy link
Collaborator Author

Do you see any problems with the above?

No, it sounds great. Sorry that it took me a year to notice that I essentially dropped this discussion.

@expez Would you take a PR? I've come across another case today where I needed this. As a refresher, here's a description of the issue:

  • I have a back-end whose doc-buffer contains formatted text, including faces, overlays, and various bits of non-essential information.

  • company-quickhelp shows all of that text, instead of showing just the relevant parts.

    Concretely, here's an example of a doc buffer:

    screenshot from 2017-04-15 01-07-14

    And here's what company-quickhelp displays it as:

    screenshot from 2017-04-15 01-06-58

And here's a description of the proposed solution

  • We'll change company-quickhelp to primarily use a backend call with argument quickhelp-string, and only fall back to doc-buffer if that quickhelp-string call returns nil. This will change nothing for existing backends, but it will allow backends to give company-quickhelp a clean, minimal string to display.

🚦 ?

@expez
Copy link
Collaborator

expez commented Apr 15, 2017

We'll change company-quickhelp to primarily use a backend call with argument quickhelp-string, and only fall back to doc-buffer if that quickhelp-string call returns nil.

Sure, that's fine.

In general, I think it would be a lot better to just clean up the doc buffer, though. I don't see why you'd want e.g. that that long file path as part of the documentation.

By cleaning up the doc buffer, all the people who prefer hitting F1 and getting a regular help popup would also benefit :)

@cpitclaudel
Copy link
Collaborator Author

In general, I think it would be a lot better to just clean up the doc buffer, though. I don't see why you'd want e.g. that that long file path as part of the documentation.

It helps you see which file a symbol was resolved from, which tends to be be an issue sometimes. Since the doc buffer is a lot larger that pop-ups, I feel that it's OK to show more information in it :)

Sure, that's fine.

PR incoming! :)

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

No branches or pull requests

3 participants