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

Move strip trailing whitespace into main code; remove plugin #1151

Merged
merged 19 commits into from
Oct 4, 2022

Conversation

jeremypw
Copy link
Collaborator

@jeremypw jeremypw commented Dec 8, 2021

Fixes #1098

A hopefully minor behavior change is that stripping (and saving) now occurs whenever the document loses focus as well as on manual saving.

Stripping does not occur on every autosave as it could interfere unacceptably with ongoing editing.

An issue raised by this is what to do about the already installed plugin - this PR overwrites it so that it does not appear in the Extensions dialog. Alternative solutions welcome.

I guess that once the app is supplied only as a Flatpak this will not be an issue and the plugin overwriting code can be removed.

@jeremypw jeremypw marked this pull request as draft December 13, 2021 16:04
@jeremypw
Copy link
Collaborator Author

Bug found - converting to draft while fixing.

@jeremypw
Copy link
Collaborator Author

Reduced scope of PR so that current behaviour (strip only on manual save or close) is unaffected. Stripping on autosave would be easy to add later if required.

@jeremypw jeremypw marked this pull request as ready for review August 11, 2022 13:47
@jeremypw
Copy link
Collaborator Author

The problem with CI appears unrelated to this PR - it compiles OK locally.

@jeremypw jeremypw requested a review from a team August 11, 2022 13:50
zeebok
zeebok previously approved these changes Aug 20, 2022
Copy link
Contributor

@zeebok zeebok left a comment

Choose a reason for hiding this comment

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

This looks good to me

src/Services/Document.vala Outdated Show resolved Hide resolved
@jeremypw
Copy link
Collaborator Author

A possible refinement is to have the "strip trailing whiltespace" functionality independent of saving so that it occurs even if "autosave" is off.

@zeebok
Copy link
Contributor

zeebok commented Aug 21, 2022

Hmmm how would that look separated from saving? Remove the trailing white space when a user starts a new line?

@jeremypw
Copy link
Collaborator Author

No, I found it too intrusive for stripping to occur immediately after leaving a line. For example, I like to insert a blank line and the closing brace of an if () {} clause then go back and fill in the body of the clause - I do not want the indenting spaces to be removed at that point. The stripping would still occur when losing focus - e.g. switching to another document or a terminal even if the document is not autosaving. At the moment with master I have to consciously press <Ctrl>S on each document before running vala-lint.

@zeebok
Copy link
Contributor

zeebok commented Aug 22, 2022

Gotcha. Do you think that might not do it often enough? If I write a quick fix, or just code for a short time, in a single doc and close Code it sounds like it wouldn't strip. Not a frequent scenario but one I can picture happening.

@jeremypw
Copy link
Collaborator Author

I think #1188 might be relevant here - this could also ensure stripping occurs.

@zeebok
Copy link
Contributor

zeebok commented Aug 22, 2022

Yeah that seems good. Personally the more places it checks and does the stripping without interfering the better so it ensures a consistent experience. Losing focus, on saves, etc are all good places to me.

@jeremypw
Copy link
Collaborator Author

@zeebok Is it OK to merge this PR (which should behave pretty much the same as master, without regressions) and make changes to behaviour in future PRs?

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

I am in favor of moving plugins into the main source for sure 👍

Instead of using the phrase "blacklist" here, can we use "blocklist" instead?

@Marukesu
Copy link
Contributor

An issue raised by this is what to do about the already installed plugin - this PR overwrites it so that it does not appear in the Extensions dialog. Alternative solutions welcome.

I think this should be handled from the package side, for source installations, we should recommend doing an sudo ninja uninstall before a new ninja install, so that old plugins get removed.

For the deb package, aren't plugins installed in the same package? so when an update occurs, they all got removed and only the ones in the new build installed?

the flatpak package aren't affected by this.

@jeremypw
Copy link
Collaborator Author

jeremypw commented Sep 1, 2022

@Marukesu I didn't realise Code has an uninstall ninja target - not all projects (e.g. Files) have one. In that case, and in view of the fact that Flatpaks are unaffected I will remove the relevant code from this PR. I am uncertain whether just updating a deb will remove old plugins though.

@danirabbit
Copy link
Member

Yeah the Deb package will cleanup old installed files like plugins so this only affects source installs

@jeremypw jeremypw merged commit c5908db into master Oct 4, 2022
@jeremypw jeremypw deleted the strip-trailing-refactor branch October 4, 2022 15:11
davidmhewitt added a commit that referenced this pull request Nov 14, 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.

Move Strip Trailing Space from the Extensions tab to Behavior or Interface
4 participants