minad / consult

:mag: consult.el - Consulting completing-read
GNU General Public License v3.0
1.14k stars 99 forks source link

Limit `consult-keep-lines` to region if active #202

Closed protesilaos closed 3 years ago

protesilaos commented 3 years ago

Hello Daniel! Can consult-keep-lines be limited to the current region, the same way keep-lines is? What this means is that if the region is active then the command only operates between the region's beginning and end, without touching the rest of the buffer.

minad commented 3 years ago

That's an interesting idea. Haven't thought of that. For now you can use narrowing, but it may make sense to add region support to consult-keep/focus lines too. There are more commands which respect if the buffer is narrowed, maybe these should be changed too? Consult-keep-lines is the most relevant function for this extension, and focus-lines should behave the same since both commands have a similar ui.

minad commented 3 years ago

See https://github.com/minad/consult/tree/keep-line-region for a first attempt. Focus seems to work but keep lines I am not yet sure. There is a problem with font-locking. But I must say, that I find the behavior with preview confusing. I am not fully sure if this should be added. Are you using keep/focus-lines with preview? Otherwise you would not have any visual feedback. Please give this branch a try.

minad commented 3 years ago

There is a simple implementation (simpler than what I have in the branch) - just directly narrow to the region via save-restriction+narrow-to-region around everything. But this will narrow the buffer during preview/input. Maybe this is not desired, but I find it less confusing.

protesilaos commented 3 years ago

Please give this branch a try.

I will do it now.

There is a simple implementation (simpler than what I have in the branch) - just directly narrow to the region via save-restriction+narrow-to-region around everything. But this will narrow the buffer during preview/input. Maybe this is not desired, but I find it less confusing.

Do you have a prototype? I am not against the idea if it delivers the intended results.

protesilaos commented 3 years ago

My early tests suggest that the branch works fine in terms of limiting its effect to the region. I guess what causes the confusion is the lack of feedback on the region's boundaries? If so, could it be possible to add line highlights at the beginning and end?

Are you using keep/focus-lines with preview?

Yes.

minad commented 3 years ago

I guess what causes the confusion is the lack of feedback on the region's boundaries? If so, could it be possible to add line highlights at the beginning and end?

Yes, this is the problem. One could add some overlays for example which show a pipe or some other kind of indicator. Do you also see the font-lock issue? This seems to be a bigger problem.

protesilaos commented 3 years ago

One could add some overlays for example which show a pipe or some other kind of indicator.

How about empty lines with something like the highlight face? So you would get two prominent lines on either end.

Do you also see the font-lock issue? This seems to be a bigger problem.

What happens here is that all faces are removed from the rest of the buffer, while the orderless faces get applied in the area where the region was:

Screenshot from 2021-02-04 18-09-36

Screenshot from 2021-02-04 18-09-50

The issue I see here is that the orderless faces are not a good fit for in-buffer use. They are okay for the standard minibuffer uses but here they can easily conflict with existing fontification. There is, of course, modus-themes-completions though that is not applicable everywhere.

minad commented 3 years ago

I am using the opiniated orderless faces which are a much better fit for this.

protesilaos commented 3 years ago

Yes, those are fine.

minad commented 3 years ago

The problem is that the narrowing region is also not necessarily at the line beginning or end, therefore the highlight line idea won't work properly I think. But maybe give it a try.

protesilaos commented 3 years ago

That is right. The way keep-lines works is to assume a partially-highlighted line as if it were completely within the region's boundaries (given the line-wise operation).

minad commented 3 years ago

I pushed a new version to https://github.com/minad/consult/tree/keep-line-region. I find the behavior of keep-lines a bit special, it only takes lines into account which are fully within the region, the first and last line may be excluded if the point is somewhere on the lines, instead of at the beginning/end. I cannot say that I like this behavior but I am also not sure if I should deviate from it.

minad commented 3 years ago

I believe that the current versions of consult-focus/keep-lines on the main branch is aligned with keep-lines. I would appreciate if you can give these functions a thorough test. I am not sure if I got it right under all circumstances. In particular the consult-keep-lines function turned out the complicated.

minad commented 3 years ago

Prot, regarding the faces I believe some improvements could be made in Modus. I am not using add-text-face-property to append the region face, such that the opinionated orderless highlighting should take precedence. But it seems that this does not work in all scenarios. Maybe you have some ideas?

minad commented 3 years ago

I believe the issue is fixed now - I observed an unfortunate interaction with hl-line-mode.

protesilaos commented 3 years ago

Hello Daniel! Sorry for the late reply: I almost missed this.

I will test everything this evening and report back to you.

minad commented 3 years ago

Thanks! Looking forward to your reply!

protesilaos commented 3 years ago

The functionality seems right to me. Further testing might reveal problems, which I will make sure to report. Thank you for your efforts!

With regard to how things stand with (add-face-text-property rbeg rend 'region t), I think it will be impossible to find a default face that works for all cases. Other plausible options are hl-line and secondary-selection, both of which could prove problematic. There are too many variables here for you to guarantee consistent results, unless you would override all faces which I think is not worth the trouble.

The variables are: the face applied to the narrowed area and all faces for the minibuffer, so not just those of orderless, but also completions-first-difference and completions-common-part. Then there may be defcustoms that affect those, as with the Modus themes.

For my part, changing the faces of completions or the region will have far-reaching implications both for the defaults and for the relevant defcustoms: the current styles are meant to work in their respective context, which unfortunately means that they are not always optimal for this scenario. From Consult's perspective, the potential conflict between the face applied to the narrowed area and the orderless/completions' faces will require concerted effort from all other theme authors to ensure a consistent experience.

Perhaps then, the easiest path forward is for you to replace region with a new consult-preview-region, which will default to inheriting from consult-preview-line. That face will be a known quantity for Consult, so theme authors can test all combinations and find an appropriate solution. For my case, this new consult-preview-region, would be a neutral background that performs well with all permutations of modus-themes-completions.

Alternatively, if you do not want a new face, just use consult-preview-line---though I will then have to edit that one.

minad commented 3 years ago

I will think about introducing a new face. Is there not a standard face which is also used for the selectrum/icomplete-vertical highlighted item? Since this face is necessarily compatible with the completion style highlighting. Why are you not using the opinionated completion style highlighting by default in modus? I prefer this over the subtle highlighting for completion.

minad commented 3 years ago

Maybe using the selectrum current line highlighting would be reasonable for consult-preview-line. Then this could also be used for the keep line region. But I like about the current style that the region highlighting does not change when consult-keep-lines is active.

protesilaos commented 3 years ago

Is there not a standard face which is also used for the selectrum/icomplete-vertical highlighted item?

Icomplete-vertical only defines one face, which is about the separator. So it does not have a distinct face for the current line. Instead it re-uses the icomplete-first-match. Now that face must also work with the horizontal layout and, thus, be in line with the standard looks of Icomplete (more below).

For Selectrum, the current line falls back to the highlight face. The default for that face, as found in M-x find-library RET faces RET does not make for a good combination with lots of colors. For the Modus themes, the highlight is a blue background that you see when you hover the mouse over a link. This is (1) too intense for the region, (2) not good for all the completion-related faces.

Why are you not using the opinionated completion style highlighting by default in modus?

This would change the defaults for Icomplete and Ido. It would also affect Ivy and Helm. The problem here is that the frameworks have two familities they belong to based on their default styles:

  1. Color only the text. This is what Icomplete and Ido do. Selectrum as well, with the exception of the selectrum-current-candidate, which is due to its original vertical layout.
  2. Color backgrounds as well. This is what Helm and Ivy do. Ivy, in addition, has four distinct colors for its matches (like orderless).

I have decided that Selectrum is closer to Icomplete than Ivy, so it follows the styles of group 1. Similarly, Orderless is supposed to work with the default completion, so it must follow the looks of the generic framework, otherwise it feels out of place.

modus-themes-completions nil means to respect what the framework authors have established as their default aesthetic. So group 1 only gets colored text, while group 2 has intense background colors as well. This is also due to the fact that Icomplete and Ido are horizontal by default, where colored backgrounds do not have the tabular-like impression you get for vertical views. For Selectrum, you will notice that with the exception of the current line, all of its default faces are reminiscent of Icomplete or standard completions'---no backgrounds.

Maybe using the selectrum current line highlighting would be reasonable for consult-preview-line.

I believe this implies that Selectrum is installed.

But I like about the current style that the region highlighting does not change when consult-keep-lines is active.

I see. So your current choice is the way to go and I am fine with that, both for the Modus themes and as a Consult user.

There are two options for the Modus themes that directly affect this, modus-themes-completions and modus-themes-region.

minad commented 3 years ago

Thank you for explaining your reasoning! I didn't know that you classify the completion systems in these two groups, I thought you unified the styles of all completion systems. But it also makes sense to follow the default style defined by the completion systems. (For me Selectrum, Ivy and Icomplete-vertical are all very similar. But I think Selectrum and Ivy are even closer regarding their internals since they don't use rotation. But I am talking about the internals here, not the representation.) I suggest we keep it as is then and continue to use the region style? But feel free to experiment, I don't want to stand in the way of potential theme improvements! As you suggest one could introduce a separate face, defaulting to 'region.

protesilaos commented 3 years ago

Thank you for explaining your reasoning!

You are welcome!

I suggest we keep it as is then and continue to use the region style?

I agree.

I don't want to stand in the way of potential theme improvements!

I had thought about using the rather ineffective :distant-foreground face property, but it would be hacky and fragile. Let's not bother with that.

As you suggest one could introduce a separate face, defaulting to 'region.

I don't think that would be necessary, but if you do it, please let me know so that I may add support for it.