Display selected newlines as whitespace in the selection highlight #296
Conversation
valadaptive
force-pushed
the
selection-whitespace
branch
from
92caeeb to
5492b94
Compare
There was a problem hiding this comment.
This looks good to me. There are a few things I'd like to see added before merging.
One nice follow-up would be something like https://code.visualstudio.com/updates/v1_37#_show-whitespace-in-selection (although I'm not sure what layer of the stack that would live at).
| let width = (cur_x - start_x).max(MIN_RECT_WIDTH); | ||
| f(Rect::new(start_x, line_min, start_x + width, line_max)); | ||
| let mut end_x = cur_x; | ||
| if line_ix != line_end_ix || (cluster_count != 0 && metrics.advance == 0.0) { |
There was a problem hiding this comment.
Previously, we would always draw if there were clusters but no advance (the second condition). However, now we only do so if there was an explicit break.
I can see two scenarios which we are in:
- That second condition is to catch this case, in which case it can be removed entirely.
- That second condition is for unrelated reasons, in which case it should instead be an or for the 860 line.
I think the former is more likely, but I haven't looked into the history.
There was a problem hiding this comment.
I looked over #170 but couldn't find where cluster_count was added. I suspect the only time a cluster will have zero advance is an explicit newline. (Maybe there are edge cases around zero-width spaces or something? I think a newline is the only time we want to display a selection rectangle.)
I'll take another look later but I think you're right and the cluster-counting stuff can be removed.
There was a problem hiding this comment.
Do we have any tests where there is a line with just a newline (i.e. a \n\n sequence)?
It's probably worth adding one.
There was a problem hiding this comment.
I'll look at adding more selection-drawing tests.
This is the behavior that I see in Chrome and in GTK and QT apps (at least on Linux). If you select the newline at the end of a line, it's displayed as some trailing whitespace:
This implements that behavior in Parley. This is an improvement over the existing behavior, which forces all selection boxes to be at least 8px wide, even if the text being selected is narrower than that (e.g. you select a single letter "i" at a smallish font size).
As stated in the TODO comment, ideally this whitespace would be as wide as the space (U+0020) character in the run's font, but we currently have no way to pass that information down.