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

Lib: fixes #1468, prevents notes with no title to break after synchronize. #1472

Merged
merged 1 commit into from May 6, 2019

Conversation

Projects
None yet
2 participants
@innocuo
Copy link
Collaborator

commented Apr 29, 2019

  • Fix: Serialize now leaves an empty line on .md file if title is empty.

  • Possible Fix in displayTitle: if there's no title, call Note.defaultTitle(item). This is so the NoteList doesn't display an empty block when note has no title. It's for display only; it doesn't modify the original note. I'm not sure if this can have any unintended consequences

unserialize() could also be fixed to prevent loss of the second line as described in #1468, but if serialize works ok, it's unnecessary.

@laurent22

This comment has been minimized.

Copy link
Owner

commented Apr 29, 2019

That's a change that touches many core parts of Joplin, including synchronisation, E2EE, import and export, so we need to understand well why we add this.

Do the tests still pass with this change? Is it possible to add a failing test for what was not previously working?

@innocuo

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 29, 2019

Honestly, tests are my weak point. I haven't yet reviewed the CliClient. I'll see what I can do, but I need to do some learning.

@laurent22

This comment has been minimized.

Copy link
Owner

commented Apr 30, 2019

Or maybe if you could add here some simple steps on how to replicate this bug that would help.

What you describe seems quite a big bug so I'm surprised it never came up before.

@innocuo

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 30, 2019

  • Have 2 devices (A and B) syncing to same source. I tested on 2 Macs and android + nextcloud.
  • Create a note on A, with title. Then remove the title.
  • Sync A to upload that note to the remote source.
  • Sync B.
    Expected: note in B should have no title
    What happens: note in B has a title. (line 1 of original note, but that line won't be in the body anymore).

If you can, check the .md file, it won't have an empty first line for an empty title. Instead, first line is the body.

All other problems mentioned in the bug report are due to this missing empty line.

@innocuo

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 30, 2019

Also:

the true fix for this problem is the changes I made to serialize()

The other change, to displayTitle() is cosmetical. That fixes the NoteList column, so that it doesn't display an empty line if the note has no title, and instead retrieves a default title (which is the first line of the body). We can leave out this if you think it might affect something. The priority fix is in serialize().

@innocuo

This comment has been minimized.

Copy link
Collaborator Author

commented May 1, 2019

I added a new commit to the PR with some tests:

  • saving, serializing, then unserializing a tag with and without a title
  • saving, serializing, then unserializing a folder with and without a title
  • saving, serializing, then unserializing notes, multiple test cases (with and without a title).

Current master branch doesn't pass these tests. The unserialized tag and folder have an undefined title. The unserialized note changes the body and the title.

If you apply this PR, all tests should pass.

@laurent22

This comment has been minimized.

Copy link
Owner

commented May 1, 2019

Well done, there was indeed a bug here and it could lead to data loss.

For the tests, the one in models_Folder and models_Tag are essentially testing the same thing (the underlying BaseItem class). So please could you remove the models_Tag test and move the models_Folder one to models_BaseItem? The test in models_Note is specific to notes so we can leave it there.

Let serialize enter an empty line for empty titles
Tests to confirm serialize/unserialize don't change body and title

check if item title exists, otherwise display default title.

added test checking serializing/unserializing Folders don't modify data

@innocuo innocuo force-pushed the innocuo:dev-lib-serialize branch from fc8e74c to 0e66ca3 May 2, 2019

@innocuo

This comment has been minimized.

Copy link
Collaborator Author

commented May 2, 2019

removed unneeded Tag test, moved Folder test to models_BaseItem, changed defaultTitle as you suggested, squashed commits.

@laurent22 laurent22 merged commit d213e4a into laurent22:master May 6, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
@@ -644,7 +644,7 @@ class BaseItem extends BaseModel {

static displayTitle(item) {
if (!item) return '';
return !!item.encryption_applied ? '🔑 ' + _('Encrypted') : item.title + '';
return !!item.encryption_applied ? '🔑 ' + _('Encrypted') : (!!item.title)? item.title + '' : Note.defaultTitle(item);

This comment has been minimized.

Copy link
@laurent22

laurent22 May 11, 2019

Owner

Turns out this broke the apps since Note is not imported in this class, nor should it be since this is the base class. I'll implement a fix for it.

This comment has been minimized.

Copy link
@innocuo

innocuo May 11, 2019

Author Collaborator

Sorry about this, I'll have to be more careful.

This comment has been minimized.

Copy link
@laurent22

laurent22 May 12, 2019

Owner

No problem, it can happen to anybody! And I should have spotted the issue earlier. For now, I've simply changed it to display "Untitled" when the title is undefined.

laurent22 added a commit that referenced this pull request May 11, 2019

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.