Dmitry Gutov writes: > Hi Spencer, > > On 16/09/2024 22:54, Spencer Baugh wrote: >> Eli Zaretskii writes: >>> I'm okay with adding a new style, per B, but why do we need to >>> deprecate emacs22 at the same time? Let users who want this new >>> behavior customize their completion styles to use this new style >>> instead of emacs22. That'd be fully backward compatible, and will >>> solve the problem for those who don't like the current behavior. >> That's fair enough, we don't need to deprecate emacs22 at the same >> time, >> we can wait until the new style has been battle-tested. >> I think a new style should replace emacs22 in the default >> completion-styles eventually, but that can wait. >> And after working on this bug for a while, I now am convinced that >> the >> new style approach is straightforward, and is the best way. (Although >> it would also work to make these changes in the emacs22 style) > > I'm not quite convinced about the "new style only" approach myself, > but anyway we can discuss a solution that could be applied to any > style, opt-in or opt-out. > >> Attached is a patch which adds a new ignore-after-point style. The fix >> for this bug is entirely contained in the differences between >> completion-ignore-after-point-all-completions (c-iap-a-c) and >> completion-emacs22-all-completions (c-e22-a-c). >> c-e22-a-c always omits the text after point, which means >> choose-completion always deletes that text, causing this bug. >> c-iap-a-c includes the text after point in some cases. Whenever the >> text after point is included, it's grayed out with a new >> completions-ignored face to indicate it was ignored. >> The cases where c-iap-a-c includes the text after point are those >> where >> the user will have further chances to edit or complete with that text: >> - When we're doing minibuffer completion and choose-completion will >> immediately exit the minibuffer, the text after point is not included, >> since the user won't get any further changes to use it, and it's >> probably garbage. >> - When we're doing completion-at-point (or certain kinds of >> minibuffer >> completion, e.g. completing a directory in filename completion), the >> text after point is included. In such cases, the user can always >> delete it themselves later, or might want to actually use it. >> To make this work consistently with completion-try-completion (which >> keeps point before the ignored suffix in both the ignore-after-point and >> emacs22 styles), choose-completion-string now checks a new >> 'completion-position-after-insert text property, and moves point to that >> position. >> (There are two other changes which are general improvements >> unrelated to >> this bug: >> - The behavior of keeping point before the ignored suffix is more >> consistent. >> - Instead of hardcoding try-completion and all-completion, >> ignore-after-point uses the configured completion styles.) > > Thank you, I see a few problems with that approach (as discussed > privately as well). To recap: > > From my POV integrating the result with company-mode is > non-trivial. And the created visual doesn't seem optimal in a number > of edge cases (long prefix = weird-looking popup; this probably > applies to the *Completions* buffer as well, though maybe to a lesser > extent). I think this is preferable, though, and even if it isn't, it should still be supported. This keeps it explicit to the user exactly what text will be inserted into the buffer upon accepting a completion. Consider these two cases of completion (| representing point): A. switch-to-|asdf B. switch-to-|buffer In both cases, switch-to-next-buffer is a potential completion, provided by "emacs22" in the case A and "basic" in case B. But in case A, if switch-to-next-buffer is chosen, the "asdf" should be preserved, and in case B, if switch-to-next-buffer, the "buffer" should be deleted. Because they have different behavior, they should appear differently to the user. That's why I think case A should show "switch-to-next-bufferasdf" in the *Completions* buffer, with "asdf" grayed out via a "completions-ignored" face. If a user decides they don't want "asdf" text to be shown in case A, then they can customize that. If a frontend decides it doesn't want to show that text, it can omit the text. But it should at least be *possible* for the text to be shown. Otherwise there's no way to distinguish the two cases. > Another problem is both the "all-completions" method and the insertion > function call out to UI: choose-completion-string--should-exit > references minibuffer-completion-table and completion-no-auto-exit > directly, for example. I'm on the fence about coupling to the > completion category. That's not an important aspect, I've removed that dependency in my latest version. > Finally, the use of 'completion-position-after-insert' seems like it > could be used separately from the "ignored text", meaning the spans > don't have to match. So it could be a separate feature, one that's > easy enough to implement on its own. Yes, and I indeed think this feature is useful on its own, because it allows choose-completion to be fully equivalent to completion-try-completion. Right now completion-try-completion can change point, but choose-completion can't. That's limiting for a bunch of reasons, and the inability to fix this bug purely in a completion style is one of them. So I think we should just go ahead and implement this feature. And if we do so, it makes it possible to fix this bug with only changes inside a completion style. > None of the above would be insurmountable, but here's what I think > avoids it using the 'completion--adjust-metadata' thingy that was > originally added for 'flex' a few releases ago: adding a metadata key > 'completion-ignore-after-point'. > > The attached patch does not make a distinction for file name > completion - it just covers the core problem - but I think any UI > could use the addition and likewise lookup the 'file' category, and > print the "hidden" suffix in the Completions, and decide to drop the > suffix in your first scenario (file name completion with exit > imminent). > > Spencer, Stefan, WDYT? Your patch is simple, and it works for default completion, but two issues: - Your patch doesn't distinguish the two cases A and B that I described above. - Your patch will require company-mode and other frontends to change. My patch does not strictly require that. But if we're already requiring that, I think we should take the opportunity to add the more powerful feature completion-position-after-insert. Here's a simplified version of my earlier patch, which modifies the emacs22 style to make it easier to discuss. I think this is equally simple as your patch, but it: - Distinguishes the cases A and B by including the ignored text in the completion, just grayed out. - Fixes the bug for other frontends without requiring them to change (they get additional benefit when they change to support completion-position-after-insert) Note that due to a separate bug in completion--twq-all, used in filename completion, the graying-out face is dropped from the completion candidates before they reach *Completions*; so if you try this, try it by e.g. completing Lisp symbols.