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

Don't add and trim trailing newlines from documents #388

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@alexander255
Copy link

commented Dec 18, 2018

Fixes issue #124.

@alexander255 alexander255 referenced this pull request Dec 18, 2018

Open

0x0A added to EOF #124

@lukefromdc lukefromdc requested a review from mate-desktop/core-team Dec 19, 2018

@lukefromdc

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

How do I test this?

@alexander255

This comment has been minimized.

Copy link
Author

commented Dec 20, 2018

Good point! 🙂

Here's a testing scenario:

  1. Start pluma without this patch applied
  2. Open/create a text file, write a single line of text with no line breaks and save the document
  3. Close pluma
  4. Open the text file in a hex editor (such as ghex)
  5. Observe that additionally to the content you wrote a newline character (0x0A) was appended to the contents
  6. Close the hex editor again
  7. Open the same text file in patched pluma with these changes applied
  8. Observe that the final newline is now visible
  9. Remove the final newline and save again
  10. Close pluma again
  11. Open the text file in the hex editor again
  12. Observe that now only the content was written without any extra characters
@lukefromdc

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

With pluma from master I saved this file (with no newline in it)
1234566789010
and got this hexdump from it:

0000000 3231 3433 3635 3736 3938 3130 0a30     
000000e

@alexander255

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

The output is correct, but you apparently set the Hex editor to 16-bit little endian 🙂
See how it groups each set of two characters (16-bit) and reverses their order (little endian)? Try setting it to 8-bit and it should be more obvious what is happening!

@alexander255

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

Also it appears I wrote some nonsense into my scenario: The newline character is of course 0x0A (=10d), not 0x10 (= 16d). Sorry bout that!

@lukefromdc

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

I don't know enough about this to properly evaluate this PR. @monsta ? @raveit65 ?

@lukefromdc
Copy link
Member

left a comment

While I do not understand enough about hexdump, hex editors, or the file formats here to properly evaluate this PR by a hexdump of a test file, this builds and runs fine here, and gave these results on a simple test file:

BEFORE(master):
12345678

yields hexdump

0000000 3231 3433 3635 3837 000a               
0000009

AFTER(this PR):
12345678
yields hexdump

0000000 3231 3433 3635 3837                    
0000008

This needs review by someone who fully understands it

@lukefromdc lukefromdc requested a review from mate-desktop/core-team Dec 22, 2018

@lukefromdc

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

Also note that for testing your test files need to be created with the version of pluma being tested

@alexander255

This comment has been minimized.

Copy link
Author

commented Dec 23, 2018

@lukefromdc: Thanks for trying to review this!

A note regarding your last comment: You can also create the testing files without switching pluma version (printf 'bla\n' > test-file vs printf 'bla' > test-file), but then you cannot test the whole load+edit+save cycle making it rather useless.

@lukefromdc

This comment has been minimized.

Copy link
Member

commented Dec 23, 2018

@monsta @raveit65 , are either of you able to properly evaluate this?

@raveit65

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

Weird, i followed your instructions and wrote Hello world to a new file with pluma.
In ghex i dont see a new line, but i see a dot.
ghex
But this dot or a new line isn't visible in pluma after opening the file with ghex.

@alexander255

This comment has been minimized.

Copy link
Author

commented Dec 28, 2018

The dot simply means „non-printable character“ and is inserted for everything it cannot easily print as ASCII (including things like NUL-bytes, tabs, newline/carriage return, anything above 128, …). On the left column you see that the dot was inserted for a 0x0A, which is ASCII newline, so everything working as expected. 🙂

@raveit65

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

Ok that works, with PR the newline is visible in pluma and can be removed.
But why this is a problem?
And maybe we can follow gedit upstream and remove the whole file, as all is now GtkSourceView?
See https://gitlab.gnome.org/GNOME/gedit/commit/f7607b420e52836b5c177d3835185bcf562fb7ac
@monsta @sc0w
Thoughts?

@raveit65 raveit65 requested a review from mate-desktop/contributors Jan 21, 2019

@sc0w

sc0w approved these changes Jan 22, 2019

Copy link
Member

left a comment

It works, it fixes the weird, wrong behavior.

The new behavior is sensible to me.

mousepad, geany, nano, emacs have the same behavior like this PR

question: we need the new line at the end for some reason?

@sc0w

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

I noticed vi / vim works like pluma without this PR, and it shows a warning opening files without new line at the end:

2019-01-22_01-56

why?

we need a gsettings key to enable the new behavior of this PR?

@sc0w sc0w requested a review from mate-desktop/core-team Jan 22, 2019

@sc0w

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

and github shows warning adding new files with no new line at the end:

2019-01-22_02-32

@raveit65

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

and github shows warning adding new files with no new line at the end:

This isn't good

@sc0w

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

I agree with the PR if it adds a gsettings key

gedit has this key:

2019-01-22_10-19

@sc0w

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

git diff also shows warning with no new line at the end:

2019-01-22_12-05

@sc0w
Copy link
Member

left a comment

gsettings key is needed

@sc0w sc0w requested a review from mate-desktop/core-team Jan 23, 2019

@vkareh
Copy link
Member

left a comment

I agree that if we want this, it needs to be behind gsettings. Keeping a newline at the end of the file is essential to having a fully-formed last line in any file. By definition, a character line ends with a newline character (\n). Without it, it's not actually a line.

Keep in mind that in Unix, streams are text, if you were to pipe multiple text streams, and the last line of any file is missing its newline character, you will end up concatenating to files in the wrong location.

Example:
bad.txt

bad

good.txt

good\n

Now look at what I can do with them:

vkareh@p50:~ $ cat good.txt 
good
vkareh@p50:~ $ cat bad.txt 
badvkareh@p50:~ $
vkareh@p50:~ $ cat good.txt bad.txt 
good
badvkareh@p50:~ $ 
vkareh@p50:~ $ cat bad.txt good.txt 
badgood
vkareh@p50:~ $
@raveit65

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Any news or progress?
Or should we close PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.