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

Strip whitespace in latexsub :exit-function #206

Closed
wants to merge 2 commits into from

Conversation

non-Jedi
Copy link
Contributor

@non-Jedi non-Jedi commented Mar 15, 2024

According to the docstring for completion-extra-properties, the string passed to the :exit-function should be the bare text to which the field was completed, but helm-mode adds an extra space at the end of string. Since none of our latexsubs include whitespace, we can safely strip whitespace in the :exit-function and work around this helm bug.

I will file an upstring bug report against helm, but I think it's worth fixing quickly here since it's only a simple 3-line change.

Closes #204.

@tpapp, @giordano, or @FelipeLema could one of you review please? This fixes an extremely annoying bug for anyone using helm. @giordano, could you please double-check that this fixes the issue for you if you get a chance?

According to the docstring for completion-extra-properties, the "STRING" passed to the
:exit-function should be the bare text to which the field was completed, but helm-mode adds
an extra space at the end of "STRING". Since none of our latexsubs include whitespace, we
can safely strip whitespace in the :exit-function and work around this helm bug.

I will file an upstring bug report against helm, but I think it's worth fixing quickly here
since it's only a simple 3-line change.

Closes #204.
@non-Jedi
Copy link
Contributor Author

For anyone curious on history, this is the same bug as emacs-helm/helm#2360. I'm not at all sure how I had helm working before given that Thierry only fixed that issue for insertion but not for what :exit-function sees. Based on what I see there, an upstream solution to this bug seems unlikely, and this workaround is relatively painless.

Copy link
Collaborator

@tpapp tpapp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

string-clean-whitespace does more than is needed and is provided by subr-x instead of just
subr, so was causing tests to fail on older emacs versions.
@non-Jedi
Copy link
Contributor Author

non-Jedi commented Mar 15, 2024

Thanks @tpapp and @FelipeLema! Merged as 2dfc869.

@non-Jedi non-Jedi closed this Mar 15, 2024
@non-Jedi non-Jedi deleted the nj/fix-204-helm-whitespace branch March 15, 2024 17:30
@giordano
Copy link

Sorry, I had a chance to test this only now! It mostly works in the sense that \alph<TAB> is replaced with α straight away (yay!), but \alpha<TAB> does nothing, \alpha remains there. Thanks for looking into this!

@non-Jedi
Copy link
Contributor Author

Looks like helm-mode isn't calling the :exit-function when the symbol is already complete. I don't immediately see a way of working around this without an upstream fix. Will open an issue against helm once I've created a minimal reproducer.

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.

Unicode substitution broken with helm
4 participants