option to restore previous TAB behavior #196
Conversation
Simplify code by saving the `\` and looking up the match from the primary hash table.
|
I can be one of those "interested users":) Currently trying it out, and it seems to be working nicely :) Thanks! @tpapp ! |
julia-mode.el
Outdated
| (let ((partial (buffer-substring-no-properties beg (point)))) | ||
| (message "partial %s" partial) | ||
| (when-let (replacements (gethash partial julia-mode--latexsubs-partials)) | ||
| (message "replacements %s" (prin1 replacements)) |
There was a problem hiding this comment.
This is cool for debugging purposes, but it bloats the minibuffer unnecessarily, even if temporarily.
This and the previous message line should probably be deleted before merging.
julia-mode.el
Outdated
| (message "replacements %s" (prin1 replacements)) | ||
| (let* ((complete-match (member partial replacements)) | ||
| (replacement (cond ((and complete-match julia-latexsub-greedy) partial) | ||
| ((cdr replacements) (gethash (completing-read "LaTeX completions: " replacements) julia-mode-latexsubs)) |
There was a problem hiding this comment.
I think this is an error.
When the completion is not full, e.g., \cir, I am offered a list of valid completions. When I press enter on one of them, I get a progn: Wrong type argument: char-or-string-p, nil error.
After some inspection, it seems that this is because this line here should return the raw latex control sequence, not the already substituted value. The substitution is performed in the julia-latexsub-or-indent function.
If I change this line to
((cdr replacements) (completing-read "LaTeX completions: " replacements))then it works as expected.
|
Any chance of getting this merged soon? |
|
It still needs tests. I will try to do them soon and merge your suggestions too. I appreciate the ping. |
|
I incorporated the suggestions by @dhanak and added some basic tests. While one can always prettify this further, it is functional so I would prefer to merge and then leave bells & whistles for future work. Further testing is welcome, but a code review would be best. |
|
asking the usual suspects for review before merging, @giordano, @non-Jedi, @FelipeLema, @ajhalme |
|
|
||
| eg in your `julia-mode-hook'." | ||
| (interactive "*i") | ||
| (if-let (replacement (julia-mode--latexsub-before-point)) |
There was a problem hiding this comment.
Sorry for not reviewing before you merged. I've been meaning to dig into this for months. Could you please comment on why this entire if-let couldn't be replaced with something like:
(let ((completion-at-point-functions '(julia-mode-latexsubs-completion-at-point-before)))
(unless (completion-at-point)
(julia-indent-line)))
I'm not immediately seeing why this wouldn't give both better features and integration and significantly more implementation simplicity.
I had meant to do a mock-up PR of this implementation to let others play with it and compare with the one in this PR, but haven't had the mental energy to do so, and I'm not sure when I will, so I'm just posting the idea in case there's an obvious problem I'm missing.
There was a problem hiding this comment.
I guess this option is missing this from the OP
option for greedy replacement for a full match (eg \bar will replace, no popup)
but I think you could handle just that case with significantly less complexity than this PR.
There was a problem hiding this comment.
Sorry, I do not understand. julia-latexsub-greedy, which is set to t by default, does exactly this (eg \bar just completes).
Regarding complexity: I agree that in an ideal world, this PR would not have been necessary. But practically, the generic completion mechanisms of Emacs have proven to be baroque and full of corner cases and/or implementation bugs like #204 (thanks for chasing many of those down!). I think that for now there is value in maintaining our own version which Just Works ™️.
| (progn | ||
| (delete-backward-char (- (point) (car replacement))) | ||
| (insert (gethash (cdr replacement) julia-mode-latexsubs))) | ||
| (julia-indent-line))) |
There was a problem hiding this comment.
Wait, if this ultimately calls julia-indent-line, I don't think it solves the problem from #193 at all. The issue in #193 was being able to have TAB work for both latex substitution as well as inserting TABs (see example in this comment). I think this might do what ronisbr was looking for if it called indent-for-tab-command instead of julia-indent-line.
There was a problem hiding this comment.
Sorry, I missed this comment, just found it among messages. This PR solves the problem for me, if anyone still has problems with it please open another issue.
Specifically, it only calls julia-indent-line when it could not make a substitution. So, TAB tries to subtitute, and indents if it can't. I find that behavior useful.
There was a problem hiding this comment.
So basically it gives you the reverse of the behavior of indent-for-tab-command when tab-always-indent is set to 'complete:
- With
indent-for-tab-commandwith(setq tab-always-indent 'complete): indent then complete - With this function: complete (but only latexsubs) then indent
If we change julia-indent-line in this function to indent-for-tab-command, the user can change between TAB inserting tabs and TAB indenting the line by customizing the value of tab-always-indent. I'll create a PR for your review when I have a few minutes.
If you haven't seen it yet, could you please comment on my other review comment when you get a chance?
Fixes #193.
This is preliminary, comments are welcome, even along the lines of "Tamás your elisp code is so horrible that I had to wash my eyes with soap after reading it, here is how you do it nicer".
Try it with
Maybe we can add an option for it.
Possible improvements:
completing-read? is there a nicer popup version?completing-read\barwill replace, no popup)Have to do before merging this: