Opened 10 years ago

Closed 9 years ago

Last modified 6 years ago

#1765 closed defect (fixed)

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

Reported by: erikos Owned by: erikos
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: 0.84.x
Severity: Critical Keywords: r+, olpc-0.84
Cc: dsd, tomeu, sayamindu Distribution/OS: Unspecified
Bug Status: New

Description

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.

Attachments (2)

1765_sync_ds_0.84.patch (2.4 KB) - added by erikos 10 years ago.
1765_sync_ds_0.84_v2.patch (2.4 KB) - added by erikos 10 years ago.
Patch that does remove the signal handler on new assignment

Download all attachments as: .zip

Change History (17)

Changed 10 years ago by erikos

comment:1 Changed 10 years ago by erikos

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

comment:2 Changed 10 years ago by erikos

  • Keywords olpc-0.84 added

Changed 10 years ago by erikos

Patch that does remove the signal handler on new assignment

comment:3 Changed 10 years ago by erikos

  • Cc tomeu added

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.

comment:4 Changed 10 years ago by sayamindu

  • Keywords r+ added; r? removed
  • Resolution set to fixed
  • Status changed from new to closed

Thanks for the patch - applied as 0fc8157a442c0317edcf21d93503efa2de6a55c1

comment:5 Changed 10 years ago by sayamindu

  • Keywords olpc-0.84 removed

comment:6 Changed 10 years ago by sayamindu

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:7 Changed 10 years ago by sayamindu

  • Keywords r? added; r+ removed
  • Owner changed from sayamindu to erikos
  • Status changed from reopened to assigned

comment:8 Changed 10 years ago by sayamindu

  • Cc sayamindu added

comment:9 Changed 10 years ago by 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

comment:10 follow-up: Changed 10 years ago by erikos

  • Keywords olpc-0.84 added
  • Milestone changed from 0.84 to 0.88

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.

comment:11 in reply to: ↑ 10 Changed 10 years ago by 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).

comment:12 Changed 9 years ago by erikos

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:13 Changed 9 years ago by erikos

  • Keywords r+ added; r? removed

comment:14 Changed 6 years ago by dnarvaez

  • Component changed from sugar-toolkit to Sugar

comment:15 Changed 6 years ago by dnarvaez

  • Milestone 0.88 deleted

Milestone 0.88 deleted

Note: See TracTickets for help on using tickets.