Skip to content

Render whitespace#1284

Merged
dzhou121 merged 3 commits intolapce:masterfrom
Nykseli:render-whitespace
Sep 21, 2022
Merged

Render whitespace#1284
dzhou121 merged 3 commits intolapce:masterfrom
Nykseli:render-whitespace

Conversation

@Nykseli
Copy link
Copy Markdown
Contributor

@Nykseli Nykseli commented Sep 19, 2022

Adding simple logic for whitespace rendering. Rendering logic is similar to VSCode.
Notably this doesn't take selected text into account. So this doesn't implement the selection whitespace rendering option.

Feature request: #249

Render whitespace with similar logic to VSCode.
Notably this doesn't take selected text into account.
Copy link
Copy Markdown
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this looks promising!

I have one nit to pick: would it be possible to replace the "push-in-loop" style appending with string.extend(iter) instead?

I'm also cc'ing #302 - if we'll ever have dropdowns, we could think about turning the setting into an enum and avoid string comparisons.

@Jerrody

This comment was marked as off-topic.

@Nykseli
Copy link
Copy Markdown
Contributor Author

Nykseli commented Sep 20, 2022

I have one nit to pick: would it be possible to replace the "push-in-loop" style appending with string.extend(iter) instead?

Line 2027:

for c in whitespace_buffer.iter() {
    rendered_whitespaces.push(c);
}

And line 2043:

whitespace_buffer
    .iter()
    .for_each(|c| rendered_whitespaces.push(*c));

Can be easily turned into

rendered_whitespaces.extend(whitespace_buffer.iter());

And I agree that it's a lot cleaner that way. Would that be enough? I think removing other "push-in-loops" require rewriting the whole logic.

we could think about turning the setting into an enum and avoid string comparisons.

I agree with using enums. They are easy to implement, easy to understand and the comparisons are fast. And they even work well if/when the UI starts supporting translations.

@Jerrody
Copy link
Copy Markdown
Contributor

Jerrody commented Sep 20, 2022

I think removing other "push-in-loops" require rewriting the whole logic.

Why do you think that it will be required.

@bugadani
Copy link
Copy Markdown
Contributor

I think removing other "push-in-loops" require rewriting the whole logic.

Not really, I was only referring to those 3-liner snippets you quoted (I saw 3 of those I think). So yup, that's probably enough here :)

@Jerrody

This comment was marked as off-topic.

@Nykseli
Copy link
Copy Markdown
Contributor Author

Nykseli commented Sep 20, 2022

Added a commit that uses extend where it makes sense.

I saw 3 of those I think

There was 3 but one of those was pushing n amount of empty spaces (I actually copied that snipped in my previous comment) and I don't think there is cleaner/faster way to add n amount of spaces at the end of a string. I might be wrong though.

@dzhou121 dzhou121 merged commit c5a924e into lapce:master Sep 21, 2022
oknozor pushed a commit to oknozor/lapce that referenced this pull request Sep 25, 2022
* Render whitespace

Render whitespace with similar logic to VSCode.
Notably this doesn't take selected text into account.

* Make clippy happy

* Use extend instead of pushing in loops
This was referenced Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants