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

Fix: Normalize line break when exporting notes #2819

Merged
merged 2 commits into from
Apr 15, 2021

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Mar 31, 2021

Fix

Fixes #2646

Context

Depending on the platform (Windows/MacOS), the monaco editor uses a different default line break (EOL) (check documentation). I did some tests creating notes on Windows/MacOS and I verified that notes have a different line break.

This shouldn't be a problem but I realised that for notes created on Windows, when they're exported the line breaks are not properly transformed, here is an example with a couple of notes, each one created in a different platform:

MacOS:
Screenshot 2021-03-31 at 19 42 42

original text: "MacOS\nLine 1\nLine 2\n\nLine 3"
exported text: "MacOS\r\nLine 1\r\nLine 2\r\n\r\nLine 3"

Windows:
Screenshot 2021-03-31 at 19 42 37

original text: "Windows\r\nLine 1\r\nLine 2\r\n\r\nLine 3"
exported text: "Windows\r\r\nLine 1\r\r\nLine 2\r\r\n\r\r\nLine 3"

As you can see in the Windows example, the \r\n sequence is transformed to \r\r\n which can lead to issues depending on how the app interprets the \r char. Here are some examples on how different apps on Windows handle this case:

Solution

My approach has been to create a normalize function that first converts all \r\n into \n chars and then does a second pass and converts all \n into \r\n. This way we keep all the exported notes with CRLF line break format.

Test

  1. Create a note with the single line break and double line breaks on the Windows Electron app
  2. Export the note through File > Export Notes...
  3. Create a note with the single line break and double line breaks on the MacOS Electron app
  4. Export the note through File > Export Notes...
  5. Check the source/notes.json file from the ZIP files (exported notes) and verify that the content of all notes have CRLF line break (\r\n)
  6. On Windows, extract the notes and open up the .txt file with Notepad
  7. Observe that no extra line breaks can be seen on Notepad
  8. Open up the same .txt file with WordPad, observe that no line break show up on the note
  9. Open the same .txt file with Notepad++ (or a text editor that shows hidden characters), observe that there are only CRLF line breaks

Release

Fixed extra line breaks issue when exporting notes

@fluiddot fluiddot added the bug Something isn't working label Mar 31, 2021
@fluiddot fluiddot self-assigned this Mar 31, 2021
* @param content string Content to normalize
*/
const normalizeLineBreak = (content: string) =>
content.replace(/\r\n/g, '\n').replace(/\n/g, '\r\n');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be done in one pass by using negative lookbehind in the regular expression: (?<!\r)\n. Unfortunately this expression is not supported yet in all browsers so it's not safe to use it.

Copy link
Member

Choose a reason for hiding this comment

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

performance here won't be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

though if you wanted to do it in one RegExp operation, you can do that with an alternation group, though that's not likely to be faster

content.replace(/\r\n|\n/g, '\r\n');

this matches \r\n first and essentially leaves it the same, but if that match fails, it then matches on a single newline and adds the leading '\r'

@fluiddot fluiddot marked this pull request as ready for review April 7, 2021 11:28
@fluiddot fluiddot requested a review from a team April 7, 2021 11:30
@dmsnell
Copy link
Member

dmsnell commented Apr 8, 2021

did you consider only sending \n for all newlines and simply stripping away \r? I think we're only normally dealing with \r\n on Windows, and if we want it to remain, should we consider preserving a platform-dependent behavior since those expectations are different between Windows, Unix/Linux/macOS, and older versions of Mac OS?

@fluiddot
Copy link
Contributor Author

fluiddot commented Apr 8, 2021

did you consider only sending \n for all newlines and simply stripping away \r? I think we're only normally dealing with \r\n on Windows, and if we want it to remain, should we consider preserving a platform-dependent behavior since those expectations are different between Windows, Unix/Linux/macOS, and older versions of Mac OS?

That's a very good question, at the beginning I thought to use an EOL platform specific but I saw that we were using CRLF in all cases so I decided to keep it (here is the PR that introduced it as a reference). Not sure if CRLF produces less incompatibilities between text editors, I'll run some tests on different platforms to see what would be the best option for this.

@fluiddot
Copy link
Contributor Author

fluiddot commented Apr 8, 2021

As far as I checked, looks like CRLF is more supported than LF in MacOS and Windows, here are the results:

CRLF

  • MacOS
    • VisualStudio ✅
    • TextEdit ✅
    • Notes ✅
  • Windows
    • NotePad ✅
    • NotePad++ ✅
    • WordPad ✅

LF

  • MacOS
    • VisualStudio ✅
    • TextEdit ✅
    • Notes ✅
  • Windows
    • NotePad ❌
    • NotePad++ ✅
    • WordPad ✅

My only concern about using a different EOL for each platform is that won't be portable, based on the previous results, if we export the notes on MacOS and then we open them in NotePad on Windows will have wrong line breaks 🤔 .

@dmsnell
Copy link
Member

dmsnell commented Apr 9, 2021

My only concern about using a different EOL for each platform is that won't be portable, based on the previous results, if we export the notes on MacOS and then we open them in NotePad on Windows will have wrong line breaks 🤔 .

I'm not sure this is really about Simplenote. The exports aren't broken - what's broken is that different platforms have different expectations for line breaks in files. Unfortunately this affects basically all software ever written. What I would guess is most important is not mixing line-endings inside the same file. Most editors will infer the line-ending style and try to match it, which is probably what Monaco is trying to do.

I think that LF is more standard around the world for exporting but that might not be true; it might be biased by my strong Unix/Linux/macOS usage. Fun fact, Macs used to use only \r as line endings. If it helps reduce friction I guess always sending \r\n is a good thing.

@fluiddot
Copy link
Contributor Author

fluiddot commented Apr 9, 2021

My only concern about using a different EOL for each platform is that won't be portable, based on the previous results, if we export the notes on MacOS and then we open them in NotePad on Windows will have wrong line breaks 🤔 .

I'm not sure this is really about Simplenote. The exports aren't broken - what's broken is that different platforms have different expectations for line breaks in files. Unfortunately this affects basically all software ever written.

I agree, the problem is on how text editors interpret the line breaks, not Simplenote. To be honest I'd expect that each note would be exported with its own EOL, the same as if you would do it in any other text editor. However if we want to avoid potential problems regarding line breaks, it is probably worth normalizing it.

What I would guess is most important is not mixing line-endings inside the same file. Most editors will infer the line-ending style and try to match it, which is probably what Monaco is trying to do.

I don't think mixing line-endings can even happen when editing a note, the only way I managed to reproduce it is by importing a text note with CRLF and without a title. In this case, we're adding it at the top of the content with a couple of line breaks. But when the note is modified then Monaco automatically fixes all the line breaks fortunately.

I don't think mixing line-endings can even happen when editing a note, the only way I managed to reproduce it is by importing a text note with CRLF and without a title. In this case, we're adding it at the top of the content with a couple of line breaks (\n). But when the note is modified then Monaco automatically fixes all the line breaks fortunately.

Regarding Monaco and line break detection, I can confirm that it detects the EOL from the text. In fact, if you create a note on Windows it will use CRLF and if you do it on MacOS it will use LF, in both platforms Simplenote will show the notes properly independently on the EOL of each note.

I think that LF is more standard around the world for exporting but that might not be true; it might be biased by my strong Unix/Linux/macOS usage. Fun fact, Macs used to use only \r as line endings. If it helps reduce friction I guess always sending \r\n is a good thing.

I also have the impression that LF is more standard but as you comment, looks like CRLF causes less friction. Another fun fact about \r (CR) is that Monaco interprets it as CRLF, actually it's not even included in its EOL enumeration.

@fluiddot fluiddot requested a review from sandymcfadden April 9, 2021 16:29
@sandymcfadden
Copy link
Contributor

Thanks for all the research on this!
I had done a bit of testing on this issue before and experimented with line endings a bit on Windows and Mac. I agree that it does seem to me like CRLF for exports causes the least friction and can be interpreted properly on most software across platforms.

Copy link
Contributor

@sandymcfadden sandymcfadden 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. As I mentioned in my other comment I agree that CRLF seems to be a good option to normalize the exports.

@fluiddot fluiddot changed the title Normalize line break when exporting notes Fix: Normalize line break when exporting notes Apr 12, 2021
@fluiddot fluiddot added this to the 2.10.0 milestone Apr 12, 2021
@fluiddot
Copy link
Contributor Author

@dmsnell from your last comment I understand you agree too on using CRLF to normalize the exports, right? Let me know if you have any other concern about this change, if not I'll merge the PR, thanks!

@fluiddot fluiddot merged commit 9e5dee4 into develop Apr 15, 2021
@fluiddot fluiddot deleted the fix/export-extra-line-breaks branch April 15, 2021 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra line breaks on exported notes
3 participants