GNOME Bugzilla – Bug 786239
Content of notes is lost after closing Bijiben (Nextcloud)
Last modified: 2018-01-25 12:37:34 UTC
Steps to reproduce: 1. Add a new notes with Bijiben / change an existing note 2. Close Bijiben 3. Start Bijiben 4. The title is already existent but the content is lost. This behaviour appears only when I'm using Bijiben with my nextcloud - local storage works perfect. In the meantime - before closing Bijiben - notes are saved correctly on my nextcloud. archlinux bijiben 3.24.1 nextcloud 11.03
Hi Tim, thanks for your bug report. I started to debug this issue, but I got stuck in https://bugzilla.gnome.org/show_bug.cgi?id=770439. We'll continue to work in this issue for next release.
Created attachment 367276 [details] [review] own-cloud-note: Fix saving notes Owncloud integration is buggy. Saving notes action happens multiple times, mainly when changing notes titles in an async way. That triggers many file update actions in paralell and, in case of title change, causes notes files being lost in the process. This patch makes all file saving actions to be in sync, to make sure that all operations will happen in order, preventing loosing data. That may increase the saving time due to operations being in sync, but it also reduces the number of saving actions.
*** Bug 739529 has been marked as a duplicate of this bug. ***
*** Bug 747556 has been marked as a duplicate of this bug. ***
Review of attachment 367276 [details] [review]: Thanks for your work on this. Anyway, the patch requires some more changes. Applying the patch results in segfault when creating a new note in nexcloud. This was tested in jhbuild How to reproduce: 1. Set the location of note (in preferences) to NextCloud 2. Create a new note. Add some content 3. Press back button, and wait a little Result: Segfault
Created attachment 367313 [details] [review] own-cloud-note: Fix saving notes Owncloud integration is buggy. Saving notes action happens multiple times, mainly when changing notes titles in an async way. That triggers many file update actions in paralell and, in case of title change, causes notes files being lost in the process. This patch makes all file saving actions to be in sync, to make sure that all operations will happen in order, preventing loosing data. That may increase the saving time due to operations being in sync, but it also reduces the number of saving actions.
Review of attachment 367313 [details] [review]: So far, the patch seems good. As the saving is done sync, it seems to block the UI longer, and this can cause frustration to people with slow internet. But I think it is better than doing the wrong thing. Anyway, we can look into improving this again later. Otherwise, lgtm (changes to be made aside). What do you think? ::: src/libbiji/provider/biji-own-cloud-note.c @@ +122,3 @@ BijiItem *item; BijiOwnCloudNote *ocnote; + g_autoptr (GFileInfo) file_info = NULL; Nitpick: The g_autoptr changes already committed to repo doesn't follow a space b/w g_autptr and paren. I myself followed the style used in gnome-builder. May be we can stick on to any one style. @@ -136,3 +131,3 @@ ocnote = BIJI_OWN_CLOUD_NOTE (note); - file = ocnote->location; - file_info = g_file_query_info (file, "time::modified", G_FILE_QUERY_INFO_NONE, NULL, NULL); + file_info = g_file_query_info (ocnote->location, "time::modified", + G_FILE_QUERY_INFO_NONE, NULL, NULL); As you are already changing the code, may be G_FILE_ATTRIBUTE_TIME_MODIFIED can be used instead of "time::modified". The documentation for the later may be hard to find, especially for newcomers. @@ +164,3 @@ + const gchar *str; + g_autoptr (GError) error = NULL; + g_autoptr (GFileOutputStream) new_file = NULL; spaces before parens may be removed. @@ +174,3 @@ + { + folder = biji_own_cloud_provider_get_folder (self->prov); + self->location = g_file_get_child (folder, self->basename); self->location may be non NULL. a g_clear_object() shall be nice to not leak it. @@ +177,3 @@ + new_file = g_file_create (self->location, G_FILE_CREATE_NONE, + NULL, &error); + if (!new_file && error) g_file_create() returns NULL on error, which means you can check for either of them, not both are required. A simple if (error) may be enough. @@ +191,3 @@ + self->location = g_file_set_display_name (self->location, self->basename, + NULL, &error); + if (!self->location && error) The same as above, check for one case. @@ +207,3 @@ + G_FILE_CREATE_REPLACE_DESTINATION, NULL, + self->cancellable, &error) && + error) "&& error" may be redundant as FALSE is return on errors.
Review of attachment 367313 [details] [review]: Forgot a nitpick. :) ::: src/libbiji/provider/biji-own-cloud-note.c @@ +186,3 @@ + + /* if file exists and it's title was changed */ + else if (self->needs_save) The empty line before this "else if" may be removed. We shall stick to gtk+ coding style.
+ folder = biji_own_cloud_provider_get_folder (self->prov); + self->location = g_file_get_child (folder, self->basename); > self->location may be non NULL. a g_clear_object() shall be nice to not leak it. g_object_unref() may be better here, as we are assigning the change immediately.
Created attachment 367395 [details] [review] own-cloud-note: Fix saving notes Owncloud integration is buggy. Saving notes action happens multiple times, mainly when changing notes titles in an async way. That triggers many file update actions in paralell and, in case of title change, causes notes files being lost in the process. This patch makes all file saving actions to be in sync, to make sure that all operations will happen in order, preventing loosing data. That may increase the saving time due to operations being in sync, but it also reduces the number of saving actions.
Review of attachment 367395 [details] [review]: lgtm. Please push to master.
With this patch, you shouldn't loose notes anymore. We'll need to review the whole owncloud thing in order to speed it up, but that will be another bug, so I'm closing this one now. Thanks.
Review of attachment 367395 [details] [review]: Pushed to master.