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

Use a hash table to look up where symbols end. #1

Closed
wants to merge 5 commits into from

Conversation

tpapp
Copy link

@tpapp tpapp commented Jan 23, 2023

No description provided.

@non-Jedi
Copy link
Owner

Still need to review this when I get a chance, but for reference since I don't know the easiest way to introspect memory size of hashtables in elisp, the equivalent Julia Set takes up 0.87 MB which is probably acceptable on computers that can comfortably run Julia in the first place. By comparison, the merged Dict for latex_symbols and emoji_symbols is only 0.37 MB.

@tpapp
Copy link
Author

tpapp commented Jan 24, 2023

(require 'memory-report)
(memory-report-object-size julia-mode--latexsubs-partials)
1031312

around 1MB which is acceptable, though admittedly not tiny.

I just wrote something quick and dirty to get your PR merged, we can always make it optional, or optimize this bit if this becomes a concern.

@tpapp
Copy link
Author

tpapp commented Feb 1, 2023

@non-Jedi: given that your PR has been going on for 3 years, it may seem churlish of me to ask you again to look at this, so just please tell me if you don't expect to have time for it in the near future, then I will merge this as is and merge your PR.

@non-Jedi
Copy link
Owner

non-Jedi commented Feb 1, 2023

Sorry. Time's been really tight lately. I hope to get to this PR this weekend at the latest but can't make any promises.

My only concern is with you saying that the latest commit I pushed didn't fix the overly aggressive \hat_mean problem (locally for me it was working entirely correctly). Even with how I understand this PR works, there's still the potential for things to go wrong in the same way. Could you test whether completing \la|mbda to \langle doesn't eat the "mbda" with this PR. I don't have the computer I'm sitting at right now setup to easily develop emacs packages unfortunately so can't quickly test this myself.

To be clear, completing \la|mbda to \langle should result in ⟨mbda rather than in just .

@tpapp
Copy link
Author

tpapp commented Feb 1, 2023

On \la|mbda, it gets completed to λ because the whole expression is consumed (this is the intention of the PR).

We could customize this, if you prefer it to stop before the point.

(No time pressure, I am busy too, and fully understand)

@non-Jedi
Copy link
Owner

non-Jedi commented Feb 1, 2023

On \la|mbda, it gets completed to λ because the whole expression is consumed (this is the intention of the PR).

We could customize this, if you prefer it to stop before the point.

(No time pressure, I am busy too, and fully understand)

Hm. Could you check what your completion-styles is set to? And then could you try again with (setq completion-styles '(emacs22))? To confirm, the behavior you're seeing is that the completion UI doesn't pop up at all, and the snippet is directly completed to λ?

To be clear, I don't have an issue with how this PR works and don't think we need to add any customization options. I just want to make sure our implementation is robust for all possible user completion-styles which we can't really control.

@tpapp
Copy link
Author

tpapp commented Feb 1, 2023

Set to (basic partial-completion emacs22).

If I set it to '(emacs22), what happens is that one TAB makes it \lambda|bda.

@non-Jedi
Copy link
Owner

non-Jedi commented Feb 1, 2023

Perfect. That means my commit adding a predicate appears to be working correctly now on your machine. Still not sure why it wasn't before and why the behavior was different for you than for me.

Functionality-wise, I have no issues with this PR, but I'm not going to merge it into my branch until I have a chance to give it at least a perfunctory code review. If that takes too long, feel free to override me and merge the whole thing directly into https://github.com/JuliaEditorSupport/julia-emacs master directly.

@tpapp
Copy link
Author

tpapp commented Feb 1, 2023

No worries, I will wait, I would appreciate review. Thanks for your time; if I don't hear from you I will ping you in 2 weeks.

@tpapp
Copy link
Author

tpapp commented Feb 2, 2023

With eglot (via eglot-jl), I get no completions via this mechanism, but

(setq completion-at-point-functions
      '(julia-mode-latexsub-completion-at-point-around julia-mode-latexsub-completion-at-point-before
                                                       eglot-completion-at-point t))

is a workaround. I have no idea what is causing this, whether eglot does not conform, or the completion mechanism is designed this way. But the above order works for both symbol completion and eglot.

Copy link
Owner

@non-Jedi non-Jedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice improvement that I think makes a lot of sense. While theoretically somebody might be looking to complete \al|ph to ≌ph, I don't think it's nearly as common as looking for α. If I had more time right now, all of the comments I left here are things that I would fix up myself after merging.

If you have some time to fix up at least the things the byte compiler complains about, great! If not, I'll do so myself when I have another free moment.

julia-mode-tests.el Show resolved Hide resolved
julia-mode.el Show resolved Hide resolved
julia-mode.el Outdated Show resolved Hide resolved
julia-mode.el Outdated Show resolved Hide resolved
julia-mode.el Outdated Show resolved Hide resolved
@non-Jedi
Copy link
Owner

With eglot (via eglot-jl), I get no completions via this mechanism, but

(setq completion-at-point-functions
      '(julia-mode-latexsub-completion-at-point-around julia-mode-latexsub-completion-at-point-before
                                                       eglot-completion-at-point t))

is a workaround. I have no idea what is causing this, whether eglot does not conform, or the completion mechanism is designed this way. But the above order works for both symbol completion and eglot.

That's annoying. Fundamentally what's going on is that you can only use one thing from completion-at-point-functions at once. When there's no \-prefix, the latexsub capfs let the completion fall through to the next candidate capf in the list. On the other hand, eglot can always come up with potential completions, so it almost never lets completion fall through to the next candidate.

To me, the right way to fix this is to make sure that the latexsub capfs come before eglot in the same way you've done. It's something I can fix in eglot-jl after JuliaEditorSupport#100 is merged.

@tpapp
Copy link
Author

tpapp commented Feb 20, 2023

@non-Jedi: thanks for the review, I think the byte compiler should be happy now (at least it is on Emacs 29).

@tpapp
Copy link
Author

tpapp commented Mar 7, 2023

@non-Jedi: friendly ping: the commit about is supposed to address everything you pointed out. Please approve the workflow, then we could merge if you are OK with it.

@tpapp
Copy link
Author

tpapp commented Apr 10, 2023

@non-Jedi: friendly ping. If you don't have time to review until April 15, no worries, I will just merge as is.

@tpapp
Copy link
Author

tpapp commented May 4, 2023

Closing in favor of JuliaEditorSupport#185.

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.

2 participants