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

Is there a way to run the post-completion of the only candidate, if there's only one? #476

Closed
cpitclaudel opened this issue Feb 26, 2016 · 33 comments

Comments

@cpitclaudel
Copy link
Contributor

@cpitclaudel cpitclaudel commented Feb 26, 2016

Hey Dmitry,

This is connected to previous discussions about what to do when there's a single candidate. It would be great to have a variant of company-manual-begin that runs the post-completion of the single available, when there is one, instead of saying no completion?

I think this would be a very useful feature. The use case is this: I'm writing math with company-math-unicode. I type \Cup. I want it to be replaced by , but at the company popup is gone. If I press company-manual-begin, I get No completion found (as expected. So I delete the p, press company-manual-begin, and press RET to insert .

Is there a way to run company-math's post-completion function even if there's only one candidate, and that candidate is already in the buffer? This would make my use of company-math-unicode much easier :)

Let me know if I can contribute a patch.

Cheers,
Clément.

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 27, 2016

Hi Clément,

Isn't this #451? Is there a reason to offer this kind of feature as a separate command, instead of resolving that issue properly?

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 27, 2016

Indeed, that's the first bullet point there, essentially. I made it a separate issue for two reasons:

  • Your suggestions were enough to find a workaround for most of other two bullet points
  • I'm particularly interested in solving this one

I also had the feeling that we didn't have an immediate answer to how to fix the other one properly, so I thought looking at it as multiple separate issues might help.

In particular, we discussed the problem of the frontend not showing anything and company hogging valuable keybindings, but it may not apply here. IOW I feel that we can solve the first bullet in a restricted case while still thinking about a general solution.

What would you think of company-manual-begin at the end of a single already inserted candidate being equivalent to company-manual-begin plus RET if there had been multiple candidates? Instead of saying no completion, we'd then say single completion inserted.

  • Good: we get some functionality that we didn't have before, and we don't break any existing use cases
  • Bad: We commit to what company-manual-begin does at the end of a complete identifier. This could become a setting, though

This doesn't address the issue of what to do when the last character of a candidate is typed (#451), but that's fine here.

Opinions?

@jonleivent

This comment has been minimized.

Copy link

@jonleivent jonleivent commented Feb 27, 2016

I just stepped into this problem (as well as #451), as Clement pointed out to me.

Here's a stupid suggestion: Suppose every symbol name gets suffixed by "". Hence, Cup would have the full completion "Cup", and so would still be "live" after one types "\Cup". Would that work?

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 27, 2016

Suffixed by an empty string?

@jonleivent

This comment has been minimized.

Copy link

@jonleivent jonleivent commented Feb 27, 2016

I had an underscore as the suffix - the git rendering removed it. So, suffixed by an underscore.

@jonleivent

This comment has been minimized.

Copy link

@jonleivent jonleivent commented Feb 27, 2016

As in "\Cup_".

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 27, 2016

Indeed, I thought about this; you could use a zero-width space. But this will require a change in company-math in any case, because that breaks its post-completion function.

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 27, 2016

Yes. You'd probably be better off using a plain space, though (so that the rest of the line doesn't jump). And post-completion can remove it. So, this is one possible workaround.

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 27, 2016

@cpitclaudel

What would you think of company-manual-begin at the end of a single already inserted candidate being equivalent to company-manual-begin plus RET if there had been multiple candidates? Instead of saying no completion, we'd then say single completion inserted.

Will that make the implementation of company-manual-begin more complex without making headway toward a general solution? Then, naturally, I wouldn't like it.

I'd be much happier to see a general solution.

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 27, 2016

Will that make the implementation of company-manual-begin more complex without making headway toward a general solution? Then, naturally, I wouldn't like it.

I don't think so; we already track company-manual-action to display the no completion message.

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 27, 2016

Here's a simple example of how things could look like. We could change

      (if (or (cdr candidates)
              (not (eq t (compare-strings (car candidates) nil nil
                                          prefix nil nil ignore-case))))
          candidates
        ;; Already completed and unique; don't start.
        t))))

to

      (if (or (and company--manual-action
                   (eq t (compare-strings (car candidates) nil nil
                                          (or company--manual-prefix prefix) nil nil ignore-case)))
              (cdr candidates)
              (not (eq t (compare-strings (car candidates) nil nil
                                          prefix nil nil ignore-case))))
          candidates
        ;; Already completed and unique; don't start.
        t))))

(with an obvious refactoring to eliminate the code duplication)

This pops up a menu even if there's only one completion available, but only right after pressing company-manual-begin. Pressing company-manual-begin on a short prefix that has multiple candidates, and inserting the last few letters to write out a candidate in full, doesn't keep the popup open.

Of course I don't know if this is the right solution, or if something else would work better :)

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 27, 2016

The more I look at this, the more I think just adding a workaround to company-math might be the best.

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 27, 2016

I'd be okay with that change in behavior by itself, but this part looks worrisome (could it be simply unnecessary?):

(or company--manual-prefix prefix)

Still, it seems like the "ideal" solution won't need this special case, so maybe a workaround for company-math would be better now.

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 27, 2016

(or company--manual-prefix prefix)

company-manual-prefix is only set after the first computation of candidates (it's nil the first time company--compute-candidates` is called)

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 27, 2016

So... why is that override needed?

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 27, 2016

Hum, maybe I'm confused. Do you mean that the following would be more readable?

(and company--manual-action
        (not company--manual-prefix)
        (eq t (compare-strings (car candidates) nil nil
                               prefix nil nil ignore-case)))

I think I agree; I just didn't think about it :)

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 27, 2016

A bit better. But it's still confusing. After you explained that this variable "is only set after the first computation of candidates", and I've read that a few times, I'm kind of getting the logic. But when we look at this code later, will we still remember that?

And the moment when company--manual-prefix is assigned is not documented, and might change at any time.

(I still don't really understand why we need different logic before "the first computation".)

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 27, 2016

A bit better. But it's still confusing. After you explained that this variable "is only set after the first computation of candidates", and I've read that a few times, I'm kind of getting the logic. But when we look at this code later, will we still remember that?

I agree; I think the first version was more clear in that respect

(I still don't really understand why we need different logic before "the first computation".)

Because we don't want to change the behaviour of company-manual-begin in all cases; just when it's called on a complete prefix. In other cases, we want to do the same as we do currently (i.e. hide the popup when we reach a complete, single candidate)

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 28, 2016

I think the first version was more clear in that respect

Was it? IIUC, it still relied on us understanding when company--manual-prefix would be set.

Because we don't want to change the behaviour of company-manual-begin in all cases

I see, thanks. But it doesn't seem like company-calculate-candidates should be concerned with whether company-manual-begin was called on a complete prefix, or that it should find about that this way.

Not sure we need to have different behavior in those cases, by the way (at least, not yet). If the user has entered completion sometime previously, then interacted with Emacs to make the completion full, we exit and (usually, but not always) run post-completion. In this case, we also want to exit it right away and run post-completion, right?

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 28, 2016

Was it? IIUC, it still relied on us understanding when company--manual-prefix would be set.

Indeed, but to a limited extent only; it was robust to company--manual-prefix being set before the first call to compute-candidates, for example.

I see, thanks. But it doesn't seem like company-calculate-candidates should be concerned with whether company-manual-begin was called on a complete prefix, or that it should find about that this way.

I think I agree. Maybe the whole logic of replacing the single candidate by t should be moved out of that function.

Not sure we need to have different behavior in those cases, by the way. At least, not yet: if the user has entered completion sometime previously, then interacted with Emacs to make the completion full, we exit and (usually, but not always) run post-completion. In this case, we also want to exit it right away and run post-completion, right?

My thinking was that company-manual-begin would most often be called either:

  • To complete an incomplete prefix, for example after backspacing a few times
  • To complete an already complete prefix by running its post-completion

I only wanted to change company's behaviour in the second case. In the first case completion proceeds normally.

Note that I don't necessarily advocate this particular approach: I don't have enough data to conclude whether this is a good idea.

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 28, 2016

A separate command would work well too, btw. Something like company-I'm-feeling-lucky that would insert the first candidate, and run its post-command. Then company-coq could fall back to that if company-manual-begin throws an error.

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 28, 2016

Would a separate command really be that much easier to implement?

Maybe the whole logic of replacing the single candidate by t should be moved out of that function.

Maybe.

I only wanted to change company's behaviour in the second case. In the first case completion proceeds normally.

Would the first case suffer if the changed logic affected it as well? If yes, would it be hard to fix that without creating special-casing logic in the common code?

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 28, 2016

Would a separate command really be that much easier to implement?

I don't know :)

I only wanted to change company's behaviour in the second case. In the first case completion proceeds normally.

Would the first case suffer if the changed logic affected it as well? If yes, would it be hard to fix that without creating special-casing logic in the common code?

I think it would; I'd be surprised to see a difference between completions starting automatically due to input, and completions started manually. I'm not sure how else to implement this feature without a special-case (which company--prefix already provides, in a sense)

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 28, 2016

I'd be surprised to see a difference between completions starting automatically due to input, and completions started manually.

But there already is a difference: you don't get post-completion when there's no explicit interaction. So it should be possible to add expansion of the single unique completion to all "manual" cases, but not the automatic ones.

The difference between "entered completion sometime previously" and "entered it just now" seems more important, actually. If we simply check that the current completion is "manual", and if so, and the current prefix is "valid and unique", call post-completion and exit, the user won't be able to type out the text value of a completion candidate without having it expanded.

Maybe that's not the end of the word (we could make it easily undo-able, if it isn't now), but it seems counter-intuitive. We should review existing completion UIs on this subject.

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 29, 2016

How about this: running the visible kind of callbacks (with snippet expansion, or the like) should only happen when the user invokes one of the "explicit" commands, but not later in the same completion session, even though company--manual-action is non-nil afterwards.

As a possible implementation, there might be some new variable we'd bind locally to non-nil only during the course of those explicit commands' execution.

The logic in company-update-candidates should not depend on the above variable. So, if we user types the last character in a "unique" completion without calling any of the "explicit" commands, the completion should (maybe, if it wasn't previously on) be entered and exited without running the "visible kind of callback". This would probably require some changes to the state transition logic, possibly major ones.

Aside from the implementation details, here's a decision to make: do we consider pre-completion and post-completion to be those "visible" callbacks, and run them only in the described circumstances? Or do we introduce a new one? Both choices have potential for breakage, and would require updates in some third-party code.

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 29, 2016

How about this: running the visible kind of callbacks (with snippet expansion, or the like) should only happen when the user invokes one of the "explicit" commands, but not later in the same completion session, even though company--manual-action is non-nil afterwards.

I agree with this (do I understand correctly that pressing "RET" on a candidate is an explicit command, too)?

As a possible implementation, there might be some new variable we'd bind locally to non-nil only during the course of those explicit commands' execution.

I tried to achieve the effect described above by comparing the current prefix to company-manual-prefix

Aside from the implementation details, here's a decision to make: do we consider pre-completion and post-completion to be those "visible" callbacks, and run them only in the described circumstances? Or do we introduce a new one? Both choices have potential for breakage, and would require updates in some third-party code.

I'm not entirely sure what you mean :/ What are circumstances in which we currently run callbacks, where these callbacks wouldn't run anymore?

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 29, 2016

pressing "RET" on a candidate is an explicit command

Sure. It's the main one, actually. There might be some debate whether company-complete-common should be one, too. Or whether we should have a variation of it that doesn't trigger snippet expansion.

I tried to achieve the effect described above by comparing the current prefix to company-manual-prefix

Indeed, but you relied on an unstable detail of its lifetime (or a non-obvious one, at least). And you made company-update-candidates aware of this concern. Instead, I think company-update-candidates shouldn't deal with the "only one candidate" case at all.

What are circumstances in which we currently run callbacks, where these callbacks wouldn't run anymore?

None, I think. We'll largely retain the current behavior, plus support a new use case, without noticeable drawbacks. But the reasons why post-completion isn't called during idle completion, will change.

Hopefully, this will improve the sanity of the code, rather than adding a yet-another special case to it.

If we choose the second option (use a new backend action for "visible" callbacks, rather than pre- and post-completion), the latter two will also run during idle completion. But it seems to be a convincing argument against that choice.

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 29, 2016

Hopefully, this will improve the sanity of the code, rather than adding a yet-another special case to it.

Thanks, that all makes sense now.

If we choose the second option (use a new backend action for "visible" callbacks, rather than pre- and post-completion), the latter two will also run during idle completion. But it seems to be a convincing argument against that choice.

I agree.

@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Feb 29, 2016

Great! Regarding this:

Instead of saying no completion, we'd then say single completion inserted.

We would say "sole completion" somewhere around the place post-completion is called. If possible, that function would remember that company-candidates was nil at the beginning of the current command, and at the check something like (equals company-candidates (list company-prefix)). Could be two different functions.

@AndreaOrru

This comment has been minimized.

Copy link
Contributor

@AndreaOrru AndreaOrru commented Aug 18, 2016

Hi, is there any update on this? Or any workaround to make post-completion work in this case?

dgutov added a commit that referenced this issue Dec 12, 2018
Called with M-x company-complete, it will immediately finish, of course, and
trigger post-completion hooks.

#150 #476
@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Dec 12, 2018

Please try out the forced-completion-for-unique-match branch. M-x company-complete should do the expected thing there.

@dgutov dgutov closed this in 8576100 Dec 13, 2018
@dgutov

This comment has been minimized.

Copy link
Member

@dgutov dgutov commented Dec 13, 2018

And now merged!

Coming soon to a MELPA near you.

@cpitclaudel

This comment has been minimized.

Copy link
Contributor Author

@cpitclaudel cpitclaudel commented Feb 12, 2019

And with a two-months delay, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.