search

mate-desktop / pluma

A powerful text editor for MATE
http://www.mate-desktop.org
GNU General Public License v2.0
154 stars 66 forks source link

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

Open ntninja opened 8 months ago

ntninja commented 8 months ago

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 commented 7 months ago

@mbkma Any chance of a review?

lukefromdc commented 7 months ago

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

mbkma commented 7 months ago

A few comments from my side:

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 commented 7 months ago

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 commented 5 months ago

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