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

Add completion-at-point function for completing latexsubs #82

Closed

Conversation

non-Jedi
Copy link
Contributor

@non-Jedi non-Jedi commented Nov 8, 2019

Closes #80.

Example gif using vanilla completion-at-point via M-TAB:

completion-emacs-julia

@tpapp trying this might give you a better idea what I was going for in #80. Before this could be merged, I'd need to:

  • Fix byte-compilation
  • Remove old julia-latexsub based implementation
  • add tests
  • Remove usage of subr-x which wasn't added to emacs until 24.4
  • Override Tab to do completion by default in julia-mode

@non-Jedi non-Jedi force-pushed the latexsub-complete branch 4 times, most recently from ede359a to 158bb52 Compare November 9, 2019 03:21
@tpapp
Copy link
Collaborator

tpapp commented Nov 9, 2019

I am a bit confused, I thought that completion-at-point-functions was something to do with the minibuffer.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Nov 9, 2019

completion-at-point is what is used by many major modes bundled with emacs to give completion (although it is defined in minibuffer.el). See https://www.gnu.org/software/emacs/manual/html_node/elisp/Completion-in-Buffers.html. With this PR, even without something like company installed, you can get latex symbol completion using complete-symbol (usually bound to M-tab).

@nverno
Copy link
Contributor

nverno commented Nov 9, 2019

If tab-always-indent is set to complete, normal TAB will also call completion-at-point-functions

In the completion-at-point function, the :exit-function property is
set to replace the LaTeX string with the unicode character when
completion is finished. This should give feature-parity with current code.
@non-Jedi
Copy link
Contributor Author

I've update the gif to show what emacs vanilla completion looks like with this PR without company (actually ran it with emacs -Q).

@giordano
Copy link

The problem with completion using M-TAB is that most window manager use Alt + TAB for changing window and using the alternatives for complete-symbol (like press ESC release and press TAB, to simulate M-TAB, or C-M-i) are even less practical.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Nov 11, 2019

@giordano then users are free to rebind complete-symbol/completion-at-point to some other keybinding or set tab-always-indent to 'complete as mentioned above. I really don't think we should be overriding fundamental keybindings like TAB in this package.

Maybe a note in the README/COMMENTARY on setting up vanilla TAB completion by setting tab-always-indent would suffice?

@giordano
Copy link

giordano commented Nov 11, 2019

Or maybe set indent-line-function to julia-latexsub-or-indent without overriding the binding of TAB? Anyway, I think there are several modes already overriding binding for TAB

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Nov 11, 2019

Then you lose all the benefits of this PR wrt nicer, well-integrated completion (see the gif in the first post). But I agree that doing so would be an improvement on the current state. Would probably break ess though.

EDIT: on second thought, I think that would be worse. You're breaking the implicit contract (interface) of indent-function's expected behavior by adding on unrelated completion.

@non-Jedi
Copy link
Contributor Author

For what it's worth, this change allows completion/latexsub from the middle of symbols, so it can supercede #70.

@tpapp
Copy link
Collaborator

tpapp commented Nov 12, 2019

FWIW, I am not enthusiastic about binding M-TAB for the reasons explained by @giordano.

@non-Jedi: thanks for doing this. Please comment when you think this is ready for a review.

@non-Jedi
Copy link
Contributor Author

It's ready now. Not gonna write tests until there's a clear disposition to merge/not merge.

@tpapp
Copy link
Collaborator

tpapp commented Nov 12, 2019

Is there a way to mimic the old behavior (completions pop up automatically when applicable) with this solution? I like your code very much (it is very clean and nicely written), but would be unwilling to break current behavior.

@non-Jedi
Copy link
Contributor Author

Completions only came up with Tab before, right? Not just automatically. If tab-completion is a requirement, I think the least intrusive solution would be to add a customizable variable julia-mode-force-tab-complete set to t by default that when set would locally set tab-always-indent to complete. Would that be acceptable?

@tpapp
Copy link
Collaborator

tpapp commented Nov 12, 2019

Sorry, yes, I mixed it up with some other mode.

I am fine with any solution that

  1. gets back the previous behavior by default (TAB tries to complete, if that doesn't do anything, indent)

  2. allows the user to disable the completion part.

@tpapp
Copy link
Collaborator

tpapp commented Nov 12, 2019

Also, given your last comment in #80 (what you are after does not work, after all), what's the advantage of this PR for users? (I admit that the code is much nicer, and in case this PR is not merged in full it should be ported back).

@non-Jedi
Copy link
Contributor Author

Also, given your last comment in #80 (what you are after does not work, after all), what's the advantage of this PR for users? (I admit that the code is much nicer, and in case this PR is not merged in full it should be ported back).

What I was after did end up working. I hadn't yet found the documentation on :exit-function when I wrote the comment on #80. You can see the actual replacement happening in the gif at the top of this PR. Sorry about the confusion there.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Nov 12, 2019

  1. gets back the previous behavior by default (TAB tries to complete, if that doesn't do anything, indent)

Are you super attached to this precise algorithm? With the change I just pushed in c0ba31c, Tab tries to indent and then completes if that doesn't do anything (indent->complete instead of complete->indent). Since the cases where you're editing a buffer and want to complete without having yet indented are fairly rare (and indent->complete is a built-in emacs completion behavior), this seems acceptable to me. Even in the case where the line hasn't yet been indented, completion just takes 2 presses of Tab instead of 1.

@nverno
Copy link
Contributor

nverno commented Nov 12, 2019

FWIW the behaviour @non-Jedi describes there is what I would expect as the emacsy way. There are a some outlier modes, namely cc-mode and ess, that rebind TAB, but I think that is just because they have both been around forever, and they go to lengths to replicate the standard behaviour.

@tpapp
Copy link
Collaborator

tpapp commented Nov 13, 2019

@non-Jedi: no, I think am fine with either order, but I will test in practice. I will do the cleanup for MELPA first, then make a release, then drop support for 23, and then I am open to merging this.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Nov 27, 2019

So after living with this for a bit, it has a couple of issues in practice related to third-party packages. Neither ivy nor company consistently calls the :exit-function with the 'finished status. Ivy always uses 'exact regardless of whether the text can be further completed, so the latex is never replaced (abo-abo/swiper#2345). Company will in general call it with 'finished, but in the case that there is a longer completion, it won't even after specifically choosing an option (company-mode/company-mode#935).

So in summary:

Possible ways forward:

  • Fix company, ivy, etc.
  • When :exit-function is called with 'exact, try to find some sort of way of determining if the user intended for the completion to be finished or continue (will probably end up with code specific to every misbehaving completion framework).
  • Punt on doing the replacement through completion-at-point and instead use e.g. hippie-expand and a custom wrapper around around indent-for-tab with a changed keybinding.
  • Keep existing latexsub implementation but simply add the completion system in this PR on top of it.

@tpapp
Copy link
Collaborator

tpapp commented Nov 27, 2019

Thanks for exploring this so thoroughly.

Since we do have a working completion implementation, just keeping it may be the simplest solution.

However, I find your code very nice (much cleaner than what we currently have) and it would be great if you could port it back to this package. It is my understanding that this is essentially the last option you suggested, correct?

@nverno
Copy link
Contributor

nverno commented Nov 27, 2019

In the past I've used

(when (memq status '(sole finished))
  ;; ...

which appears to work in this case for company (don't know about ivy)

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Nov 27, 2019

In the past I've used

(when (memq status '(sole finished))
  ;; ...

which appears to work in this case for company (don't know about ivy)

Even if that works in this case, I think it's going against the "meaning" of 'sole and would make vanilla completions behave in unexpected ways.

However, I find your code very nice (much cleaner than what we currently have) and it would be great if you could port it back to this package. It is my understanding that this is essentially the last option you suggested, correct?

Sure; I could refactor out the part that's common to completion and substitution.

My preference is to first wait and see what the ivy and company upstreams say. I think that current behavior is a bug and might be quickly fixed.

One other really hacky idea I had was to append a zero-width-space to the end of all the \LaTeX strings that get searched for. Then we could dump those into the completion engine and use abbrev-mode to instantly replace any occurrences of the latex strings followed by a zero-width-space with unicode. The zero-width-space ensures that latex strings which aren't tab-completed (e.g. in docstrings) don't get forced to unicode.

EDIT: actually using abbrev-mode could probably be made to work not-hackily at all. Basically it would expand if a space was typed or if expand-abbrev was specifically called. Users always have the option of reversing it by calling unexpand-abbrev.

EDIT2: So if :exit-function can't be made to work, I'm relatively confident now that abbrev-mode is the right way forward. We'll still get LaTeX string completion from the work in this PR and then substitution will be through abbrev-mode rather than the custom code we currently have. Should simplify to some extent.

@nverno
Copy link
Contributor

nverno commented Nov 27, 2019

Note it shouldn't be hard to check if exit-function is being called from company or vanilla -- but I don't think it is necessary anyway -- do you have an example of unexpected behaviour with sole?

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Dec 4, 2019

Replying to your comment here to keep it in a more relevant place: #93 (comment)

I won't be able to use any of the code you linked unless you release it under a compatible license. ;)

@nverno
Copy link
Contributor

nverno commented Dec 5, 2019

@non-Jedi Sure thing, everything there is just public domain, but I added a license -- I had been planning on suing you after you make millions off this code in the future, but oh well :D Now, I'all actually probably just steal it back if you use and improve it.

I may have some more time in a week or two if your looking for help on any of this or smie stuff.

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Dec 5, 2019

I may have some more time in a week or two if your looking for help on any of this or smie stuff.

If you want to help take care of my annoyances for me, I would of course be mighty thankful. I tend not to have a lot of time to devote to this kind of stuff. SMIE in particular is faaarr down my list of priorities and might not be touched for months if not years (if ever).

@non-Jedi
Copy link
Contributor Author

non-Jedi commented Dec 6, 2019

Neither ivy nor company consistently calls the :exit-function with the 'finished status.

Company has fixed this on master, which as far as I know is by far the most common external in-buffer completion framework.

@tpapp
Copy link
Collaborator

tpapp commented Jan 20, 2020

@non-Jedi, I think this PR does two things:

  1. reorganize the bounds finding and indentation code really nicely, to fix the same thing as Make julia-latexsub work if point in the middle of latex symbol. #70,

  2. hook into ivy and company.

I wonder if you would be willing to port the first part as a PR to current master, with tests, etc. It would be a nice improvement. Then, as those two other packages catch up, hooking into them could be done.

@non-Jedi
Copy link
Contributor Author

So my opinion is that this PR does the following:

  1. Reorganize bounds and indentation code.
  2. Hook into emacs generic completion framework (completion-at-point) to allow completion rather than just substitution of latex codes (e.g. completion of \Lam to \Lambda). This is how ivy/company now work with this code, but it doesn't require either of those.
  3. Move the substitution code (e.g. \Lambda to Λ) from a custom implementation to hooking into completion-at-point.
  4. Change both 3 and 4 so that they occur for the symbol the cursor is in the middle of rather than just the symbol the cursor is at the end of.

1 is non-controversial. 2 is probably the same as long as we keep the existing substitution stuff. 3 probably needs to wait for ivy to fix it's crap. 4 I've become convinced is a bad idea after having used this fork as a daily driver for the past months.

The current implementation uses the string from the \ before the cursor to the cursor as the symbol to lookup. This means that you can easily go from dx = 5 to Δx = 5 by simply deleting the "d" and tab-completing \Delta|x = 5 (where | represents cursor position). With 4 implemented, you first have to insert a space and then type \Delta since \Deltaa doesn't complete. This is annoying enough, and the situations where you want to substitute a latex symbol from the middle of the string are rare enough that I don't think it's a good idea here or in #70.

non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Feb 29, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Mar 1, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Mar 3, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Mar 10, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Mar 20, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Mar 31, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Mar 31, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Apr 16, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Jul 9, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
@non-Jedi
Copy link
Contributor Author

Closing in favor of #100

@non-Jedi non-Jedi closed this Jul 10, 2020
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Jul 18, 2020
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
non-Jedi added a commit to non-Jedi/julia-emacs that referenced this pull request Dec 6, 2022
This is a version of JuliaEditorSupport#82 without the usage of :exit-function to
replace the LaTeX string with a unicode symbol.
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.

Use completion-at-point for latexsub instead of overriding TAB keybinding
4 participants