Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trim whitespace on save #2377

Merged
merged 3 commits into from
Dec 2, 2023
Merged

Trim whitespace on save #2377

merged 3 commits into from
Dec 2, 2023

Conversation

bonjorno7
Copy link
Contributor

@bonjorno7 bonjorno7 commented Nov 25, 2023

When you hit Ctrl + S with this option enabled, it will trim spaces from the end of lines, and newlines from the end of the file.
I think this not only looks nice, but actually matters, because spaces count against the file size limit.

It adds a trailing newline at the end of the file if there isn't one.
It respects the file size limit (64kb or 512kb for the pro version).
It updates the cursor position and selection appropriately.

Note: if your selection was entirely trimmed, you're left with a zero length selection.
I decided not to fix this, as the user may have created such a selection intentionally.
Shouldn't be difficult though, if you'd like me to change it.

Another note: trimming only happens with Ctrl + S, not with typing save in the console.
I could try fixing this if you want, but decided not to do it yet, because I feel it's better to only trim if you can see it happen, which you can't from the console (might make sense to only run this when you're actually in the code editor then, but haven't looked into that yet).
Another reason I didn't fix it is because it was convenient being able to save without running the trimming code while I was still developing it.

Unlike my last PR, this time I've included changes to config.tic.dat, because I now know what it's for.

Fixes: #2370

I think people who would use this would rather have it automatically every time you save, instead of having to click a button every time.
@bonjorno7
Copy link
Contributor Author

bonjorno7 commented Nov 25, 2023

Seems some platforms don't like my use of min.
I'll try to fix it.

@bonjorno7
Copy link
Contributor Author

bonjorno7 commented Nov 25, 2023

I replaced it with a ternary, so all of the builds work now.
Used a force push so these back and forth changes don't need to live in the repo history forever.

@bonjorno7
Copy link
Contributor Author

It calls history before calling update; hopefully those are the right functions in the right order.
I did find one new issue before this (which was unfortunately not fixed by the last change): if you remove text at the end of the file by selecting it with your mouse and hitting backspace/delete, then save, it will bring back the deleted text from the dead, but only if a trailing newline was added (if there was already a trailing newline, the issue doesn't happen).
I'm not sure what exactly is going on with that, but it's worth noting I guess.

@bonjorno7
Copy link
Contributor Author

bonjorno7 commented Nov 27, 2023

Okay I know what's going on.
I was under the assumption that I could rely on the rest of the file being nulled, but deleting your selection at the end of the file just sets one null and leaves the garbage data.
When my code adds a trailing newline, it can overwrite that null, meaning it's then followed by previously deleted characters until the next null.
Since apparently the file doesn't need to be zero'd to be valid, I'll just change the code to zero only one byte, and leave any garbage data after it alone.

@bonjorno7
Copy link
Contributor Author

There was even more to it: the loop didn't exit because the null check didn't happen, because the null was overwritten by a newline.
Now it stores whether we found a null at the start of an iteration, and checks it at the end of that iteration.
This way it actually exits when encountering a terminator, even if there's garbage data after it.

@bonjorno7
Copy link
Contributor Author

A friend with more C experience tells me I should use memmove rather than memcpy, because memcpy doesn't necessarily copy one byte at a time.
I'll have to contemplate whether to do that, or whether it's more efficient to create a big buffer at the start instead.
So this PR isn't quite ready to merge as I'm still figuring that out; I'll mark it as draft.

@bonjorno7 bonjorno7 marked this pull request as draft November 28, 2023 17:53
In my previous PR I didn't commit this file because I didn't know what it was for.
I understand now that this is the default config, which gets loaded when you use `config reset` in the console, so it's important that it's up to date.
I'm not too experienced with C so this was tough, but I got it working.
It trims spaces at the end of lines, and newlines at the end of the file (except for the last one, if there's space for it).
It also updates the cursor position and selection.

I increment src after the done check so that its value is correct after the loop; not actually using it after the loop in this version but just in case.
That check by the way uses a stored variable because the loop overwrites the buffer, so checking the value of end at the bottom of the loop doesn't work.
I'm using memmove instead of memcpy because the data can overlap, and this is apparently still quite fast.
I don't zero out garbage data after the terminator because it's apparently unnecessary.
After trimming whitespace it calls history and update; hopefully those are the right functions in the right order.

One small note: if your selection was entirely trimmed, you're left with a zero length selection.
I can fix this by detecting if position and selection are equal, and then setting selection to null.
I chose not to do this however, because it's easy to create a zero length selection by hand, and the user might in some cases do so intentionally.
If you want to get rid of zero length selections entirely, that should probably be fixed elsewhere, rather than in this function.

Another note: trimming only happens with Ctrl + S, not with typing save in the console.
Perhaps I should fix this, though I think it has advantages too; I think ideally the trimming should only happen when you can see it happening, which you can't if you're in the console.
Also while writing this code it was nice to be able to undo and save my file without running the trimming code, though I suppose now that the code works properly I won't need it anymore.
@bonjorno7 bonjorno7 marked this pull request as ready for review November 28, 2023 19:12
@bonjorno7
Copy link
Contributor Author

bonjorno7 commented Nov 28, 2023

I opted to go with memmove, because it's more efficient than I thought; and I suppose it doesn't matter for a little text editor anyway.
Also I changed the order of the commits, so if I need to make another change to the implementation, I'll only need to force push one commit.
That's all the issues I'm aware of fixed, so unless you find something, it's ready to merge (if you think it's a good feature).

@nesbox nesbox merged commit e37cdaf into nesbox:main Dec 2, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Trim excess whitespace
2 participants