Create user option to highlight full candidate line.

[okamsn]

This really only makes a noticeable difference when there isn't text at the right margin, but I still find it helpful.

Should anything else be changed?

[raxod502]

No, seems great to me. I hadn't even thought about this, but it's a good option to have. Want to add a changelog entry for it?

[raxod502]

Oh yes, and we should also have something in the README since this is a public user option.

[clemera]

Could we shorten this to "The default is to only highlight the displayed text"? That would be a bit easier to read/understand.

[okamsn]

It is changed.

[clemera]

Thanks, I meant leaving off the ",though if..." part, too. I don't think it's needed because " The default is to only highlight the displayed text" should be sufficient. The "though if..." only describes a detail following from this statement, what do you think?

[okamsn]

I removed it.

[clemera]

Nice, thanks!

[okamsn]

I was unsure about clarifying between a candidate's displayed text and the "not-annotation" part of the candidate, and between a line of text and the horizontal space on the screen in which the line of text is located.

Similarly, should the variable's name be changed to clarify that it applies only to the current candidate, and not some other kind of formatting done to the other candidates?

[clemera]

Similarly, should the variable's name be changed to clarify that it applies only to the current candidate, and not some other kind of formatting done to the other candidates?

Hm, maybe selectrum-highlight-current-candidate-line or selectrum-highlight-current-line? I think entire line isn't strictly true anyway when there are indices for example.

[okamsn]

After thinking about it, I feel that selectrum-extend-current-candidate-highlight is a good name that describes what is happening without being confused with something else (as far as I'm aware).

Do you think that this extending should also happen for the input line? I originally only thought about the candidates, to not be obscured by the selectrum-*-highlight faces as much.

[raxod502]

The new name seems fine to me, although maybe @clemera will have another suggestion. It seems to me like extending should probably happen for the input line as well, for consistency, unless that looks odd.

[clemera]

I also like the new name! It feels natural to me that only the input gets selected, highlighting the input line would require changes to how we insert the overlay, but I leave that to @raxod502 to decide.

[raxod502]

I'm fine with doing it either way.

[clemera]

Is there anything left you would like to add @okamsn?

[okamsn]

No, I am content with the current changes. Thank you.

[clemera]

Thank you! I just noticed the dashes for the let bound variable names, which can lead to accidentally binding dynamic var names of other packages. But with the given names and the code within the let body a collision seems very unlikely so I think it's okay in this case. We can convert those cases at some later point throughout the whole package.

[raxod502]

I just noticed the dashes for the let bound variable names

Which variable names were you looking at here?

[clemera]

formatting-current-candidate, right-margin and so on. It's very unlikely to get into conflict issues with these so it was a bit pedantic to mention them, on the other hand making it a habit to avoid dashes in let bound variables can be useful, it's a subtle issue which is easy to overlook.

[raxod502]

Fair enough. I had never thought about it before, but it's true, that could happen.