GNOME Bugzilla – Bug 772178
Data is not saved after re-editing task
Last modified: 2016-10-13 16:35:14 UTC
This is on master branch. I create a task, then I try to edit it either by setting date or description. The changes are not saved after I close gnome-todo (very frustrating) The list is a local one.
Ha. To save what I have edited, I *must* close the edit pane. I realized it by code inspection ... /**¬ * GtdEditPane::edit-finished:¬ *¬ * Emitted when the the user finishes editing the task, i.e. the pane is closed.¬ */¬ signals[EDIT_FINISHED] = g_signal_new ("edit-finished",¬
I suggested that the Task should set the priv->should_save_task boolean whenever one of its properties changes (description, date, etc) and likely will be saved when switching to a new task.
Created attachment 336778 [details] [review] edit-pane: lower indentation with early return --- Not sure if I'm breaking the code style, if that's the case feel free to drop it.
Created attachment 336779 [details] [review] edit-pane: save task after editing Before this change, the task is only saved if user closes the edit-pane which makes very easy to lose data from changes. We mark the should_save_task boolean as soon as any change happens and we will save whenever a new task is loaded.
Created attachment 336780 [details] [review] edit-pane: save loaded task on quit If we have a loaded task with changes, we should ensure to save this changes before quitting gnome-todo.
Created attachment 336781 [details] [review] edit-pane: check if binding is still valid It might not be valid on dispose: "The binding will automatically be removed when either the source or the target instances are finalized."
Review of attachment 336778 [details] [review]: LGTM
Review of attachment 336779 [details] [review]: Needs some minor improvements. ::: src/gtd-edit-pane.c @@ +371,3 @@ g_clear_pointer (&priv->priority_binding, g_binding_unbind); + g_signal_handler_disconnect (gtk_text_view_get_buffer (priv->notes_textview), priv->notes_changed); + g_signal_handler_disconnect (priv->priority_combo, priv->priority_changed); I prefer to use g_signal_handlers_disconnect_by_func() and not add fields to the private section of the class. @@ +399,3 @@ + priv->notes_changed = g_signal_connect (buffer, "notify::text", + G_CALLBACK (gtd_edit_pane__task_changed_cb), + pane); This breaks the coding style. One line per argument. @@ +413,3 @@ + G_CALLBACK (gtd_edit_pane__task_changed_cb), + pane); + Ditto.
Review of attachment 336780 [details] [review]: Why not on finalize() ?
Review of attachment 336781 [details] [review]: This is the wrong approach. I'd rather track down where the task is being finalized rather than using this kind of hack here.
Comment on attachment 336778 [details] [review] edit-pane: lower indentation with early return Attachment 336778 [details] pushed as 744b4b6 - edit-pane: lower indentation with early return
(In reply to Georges Basile Stavracas Neto from comment #9) > Review of attachment 336780 [details] [review] [review]: > > Why not on finalize() ? Well, we are releasing/saving resources from the object so I thought these fits better the dispose method as per: https://developer.gnome.org/gobject/stable/gobject-memory.html Let me know if you want me to change that, or update the commit with that info.
(In reply to Georges Basile Stavracas Neto from comment #10) > Review of attachment 336781 [details] [review] [review]: > > This is the wrong approach. I'd rather track down where the task is being > finalized rather than using this kind of hack here. I don't follow why this is wrong. It isn't the task that has been finalized as we are on dispose method at this moment. The other objects that might be already released at this point making notes_binding and priority_binding invalid.
I'll re-submit the patch with minor fixes and the other two due the need to rebase them.
Created attachment 337616 [details] [review] edit-pane: save task after editing Before this change, the task is only saved if user closes the edit-pane which makes very easy to lose data from changes. We mark the should_save_task boolean as soon as any change happens and we will save whenever a new task is loaded.
Created attachment 337617 [details] [review] edit-pane: save loaded task on quit If we have a loaded task with changes, we should ensure to save this changes before quitting gnome-todo.
Created attachment 337618 [details] [review] edit-pane: check if binding is still valid It might not be valid on dispose: "The binding will automatically be removed when either the source or the target instances are finalized."
Review of attachment 337616 [details] [review]: LGTM.
Review of attachment 337617 [details] [review]: Ok, my fault. LGTM.
Review of attachment 337618 [details] [review]: I didn't fall in love with this approach, but that's probably the best we can do.
Attachment 337616 [details] pushed as e201486 - edit-pane: save task after editing Attachment 337617 [details] pushed as 9232197 - edit-pane: save loaded task on quit Attachment 337618 [details] pushed as 44a6a47 - edit-pane: check if binding is still valid