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

Retain whether a file ended with a trailing newline optionally displaying it #687

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ntninja
Copy link

@ntninja ntninja commented Oct 17, 2023

For some reason I felt the need to rewrite #388 after all these years and the result is this.

A lot of the discussion of the old PR still applies, but instead of always having the trailing newline visible (like in that PR), having the trailing newline is now optional.

Details:

  1. The trailing newline is not displayed by default but there is a setting – with UI – that can be used to always have it displayed (addresses feedback from other PR)
  2. Files that do not end in a trailing newline when opened are saved without a trailing newline instead of forcefully adding one as is currently done
  3. Newly created (or edited previously completely empty) files are saved with an automatically added trailing newline if the setting is enabled, without one otherwise (matches current behaviour by default)
  4. Each PlumaDocument instance remembers whether a hidden trailing newline is present/expected similarly to how remembering the expected/used line ending (LF/CRLF/CR) is implemented
  5. The PR is made up of several separate smaller commits that can be reviewed independently if that is preferred

@raveit65 raveit65 requested a review from a team October 22, 2023 08:32
@raveit65 raveit65 requested a review from mbkma November 18, 2023 15:37
@raveit65
Copy link
Member

@mbkma
Any chance of a review?

@lukefromdc
Copy link
Member

lukefromdc commented Nov 18, 2023

I simply don't know anything about how to actually use this in production, but will give it a test build and run

Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

This works as intended, including adding the trailing newline to a document that did not contain one if hide-trailing-newline is not checked (defaults to checked/FALSE). Nice work including this in Pluma Preferences so it can be changed w/o having to have dconf-editor installed or using gsettings from the command line

@mbkma
Copy link
Member

mbkma commented Dec 11, 2023

A few comments from my side:

  • With this PR pluma is in an inconsistent state (setting is not applied immediately, only on re-opening the document as i understand it): something I would like to avoid
  • I do not like the wording Hide Trailing Newline: It sounds like something is there but not shown, but the newline is removed not only hidden: In gedit PR it is: Ensure Trailing Newline
  • Why do you need the trimmed-trailing-newline property? In the gedit PR it is not used: https://gitlab.gnome.org/GNOME/gedit/-/commit/fe62ab8175747298a5315aaeed53ef0c56e60817
  • In the UI there is no vertical space between title and setting
  • With the Show newlines option activated, a newline is shown even if there is no trailing \n. But that likely is an issue from that option, not from this PR.

Sorry if some of my statements are wrong, I just quickly scanned the changes :)
In general I also like this option, but it should be applied instantly - as in the gedit PR.

@ntninja
Copy link
Author

ntninja commented Dec 11, 2023

I have never seen this MR in gedit, I wrote this code entirely based on how pluma works today.

Quickly scanning that code however, I think I get the confusion here. Most importantly consider what happens when you change the setting while a document is open: In the gedit MR you linked (unless I’m mistaken) opening a document with a trailing newline, then enabling the option, then saving it will cause the file to be saved without the trailing newline (ie: the newline will be lost). In my PR the document remembers whether it has previously hidden (removed) the trailing newline and will therefor correctly add it again when the document is saved even if the setting was changed in the mean time. This state (newline removed from the editing buffer or not) is what is stored in the “trimmed-trailing-newline” property – without retaining this information Pluma would also lose the newline if the setting is changed while a document with trailing newline is open and this is a lot more problematic for Pluma with this PR since the setting is visible and reachable in the UI.

  • I do not like the wording Hide Trailing Newline: It sounds like something is there but not shown, but the newline is removed not only hidden: In gedit PR it is: Ensure Trailing Newline

As should be apparent from the previous answer, something is indeed “there but not show”: It’s just the extra information on whether the document has a trailing newline is not stored in the editing buffer (where the user would see it) but in a separate property (“trimmed-trailing-newline”).

  • With this PR pluma is in an inconsistent state (setting is not applied immediately, only on re-opening the document as i understand it): something I would like to avoid

That is indeed a valid concern: It would be nice if the hidden newline got shown/hidden as the checkbox in the settings is unchecked/checked.

So basically you’re saying I should fix this todo: https://github.com/mate-desktop/pluma/pull/687/files#diff-5c01dc24a248053550fb8911bec3b952aefb442ca20c6d0647a12c6e41346532R402-R406 😉

  • With the Show newlines option activated, a newline is shown even if there is no trailing \n. But that likely is an issue from that option, not from this PR.

I looked into that and it appears the way to fix that is to let GtkSourceView know whether the file is intended to contain a trailing newline or not using gtk_source_buffer_set_implicit_trailing_newline. In fact that property appears to be identical to how my “trimmed-trailing-newline” option works in PlumaDocument, just at a lower level…

That option got added in GtkSourceView 3.14, is that old enough for Pluma to hard-depend on it?

  • In the UI there is no vertical space between title and setting

I’ll look into this one, once it is clear whether we agree on the fundamentals.

@raveit65
Copy link
Member

raveit65 commented Feb 4, 2024

@mbkma
Any news with that PR? Can this be merged or are changes needed?

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