#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)
Change History (17)
Changed 14 years ago by erikos
comment:1 Changed 14 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 14 years ago by erikos
- Keywords olpc-0.84 added
Changed 14 years ago by erikos
comment:3 Changed 14 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 14 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 14 years ago by sayamindu
- Keywords olpc-0.84 removed
comment:6 Changed 14 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 14 years ago by sayamindu
- Keywords r? added; r+ removed
- Owner changed from sayamindu to erikos
- Status changed from reopened to assigned
comment:8 Changed 14 years ago by sayamindu
- Cc sayamindu added
comment:9 Changed 14 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: ↓ 11 Changed 14 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 14 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 14 years ago by erikos
- Resolution set to fixed
- Status changed from assigned to closed
comment:13 Changed 14 years ago by erikos
- Keywords r+ added; r? removed
comment:14 Changed 10 years ago by dnarvaez
- Component changed from sugar-toolkit to Sugar
Patch that does remove the signal handler on new assignment