Changes to a Journal entries metadata are wiped if activity is still open (0.84)

[erikos]

How to reproduce:

1) Start a new Activity from the home fav view (e.g Browse)
2) Switch to Journal
3) Favourite star it, edit the title, in detail view add description and/or tags
4) Switch back to Journal list view (i.e don't leave in Journal details view)
5) Switch to Activity instance
6) Switch back to Journal view
7) Note that all previous Journal metadata details are wiped/reset

Original description in #1358.

This one cause a lot of confusion and lost data in the Berlin Pilot. I ported the 0.88 fix back to 0.84. The only thing I don't like with the patch is that we need to create another ds interface in the datastore module (we have already one in the dbus-helpers).

Patch tested on soas-0.84.

[erikos]

Patch that does remove the signal handler on new assignment

[erikos]

From a previous conversation with Tomeu:

"...if we allow the object_id to be changed later, we should disconnect the existing signal. Another alternative is raising an exception when the object_id is tried to be changed to
something else."

The patch above does disconnect the signal. This should be done in 0.88, too.

[sayamindu]

Thanks for the patch - applied as 0fc8157a442c0317edcf21d93503efa2de6a55c1

[sayamindu]

There is a small problem with the patch. We need to set self._update_signal_match = None before calling set_object_id().

diff --git a/src/sugar/datastore/datastore.py b/src/sugar/datastore/datastore.py
index 6ff10de..7aa4d15 100644
--- a/src/sugar/datastore/datastore.py
+++ b/src/sugar/datastore/datastore.py
@@ -109,12 +109,12 @@ class DSMetadata(gobject.GObject):
 
 class DSObject(object):
     def __init__(self, object_id, metadata=None, file_path=None):
+        self._update_signal_match = None
         self.set_object_id(object_id)
         self._metadata = metadata
         self._file_path = file_path
         self._destroyed = False
         self._owns_file = False
-        self._update_signal_match = None
 
     def get_object_id(self):
         return self._object_id

[sayamindu]

I'm also getting the following traceback in the logs (with the updated patch) - on pressing the Keep button:

1269291433.264587 ERROR dbus.connection: Unable to set arguments (None,) according to signature u's': <type 'exceptions.TypeError'>: Expected a string or unicode object
1269291433.265458 ERROR dbus.connection: Exception in handler for D-Bus signal:
Traceback (most recent call last):
  File "/usr/lib/python2.6/site-packages/dbus/connection.py", line 214, in maybe_handle_message
    self._handler(*args, **kwargs)
  File "/usr/lib/python2.6/site-packages/sugar/datastore/datastore.py", line 135, in __object_updated_cb
    byte_arrays=True)
  File "/usr/lib/python2.6/site-packages/dbus/proxies.py", line 140, in __call__
    **keywords)
  File "/usr/lib/python2.6/site-packages/dbus/connection.py", line 620, in call_blocking
    message.append(signature=signature, *args)
TypeError: Expected a string or unicode object

[erikos]

Sayamindu, thanks for identifying this issue!

I found out the following: When we copy (keep as) an entry we set the object_id to None (activity.activity.copy). We do this that on next save we create a new entry. Is a bit funky imho. Anyhow, in the old code we did not disconnect the handler in this case, and therefore tried to get the properties of a none object. The following code does take care of this:

 diff --git a/src/sugar/datastore/datastore.py b/src/sugar/datastore/datastore.py
index 26655a6..e264889 100644
--- a/src/sugar/datastore/datastore.py
+++ b/src/sugar/datastore/datastore.py
@@ -139,6 +139,7 @@ class DSObject(object):
     """A representation of a DS entry."""
 
     def __init__(self, object_id, metadata=None, file_path=None):
+        self._update_signal_match = None
         self.set_object_id(object_id)
         self._metadata = metadata
         self._file_path = file_path
@@ -149,16 +150,18 @@ class DSObject(object):
         return self._object_id
 
     def set_object_id(self, object_id):
+        if self._update_signal_match is not None: 
+            self._update_signal_match.remove()
         if object_id is not None:
-            _get_data_store().connect_to_signal('Updated',
-                                                self.__object_updated_cb,
-                                                arg0=object_id)
+            self._update_signal_match = _get_data_store().connect_to_signal( \
+                    'Updated', self.__object_updated_cb, arg0=object_id)
+
         self._object_id = object_id
 
     object_id = property(get_object_id, set_object_id)
 
     def __object_updated_cb(self, object_id):
-        properties = _get_data_store().get_properties(self.object_id,
+        properties = _get_data_store().get_properties(self._object_id,
                                                       byte_arrays=True)
         self._metadata.update(properties)
 

This fix should go into 0.88, too.

[sascha_silbe]

Replying to erikos:

+ self._update_signal_match = _get_data_store().connect_to_signal( \
+ 'Updated', self.object_updated_cb, arg0=object_id)

Style nitpicks: The backslash is not necessary at this point and the indentation doesn't seem to be four spaces.
Looks good to me otherwise (just visual inspection; haven't tested the patch).

[erikos]

Did land.

 http://git.sugarlabs.org/projects/sugar-toolkit/repos/mainline/commits/e497eb15baea8b7f7429fb5c5cf34e45a83aeeed