GTG

Merge lp://qastaging/~pieterleclerc-deactivatedaccount/gtg/noarchivedlists into lp://qastaging/~gtg/gtg/old-trunk

  1. noarchivedlists
  2. Merge into old-trunk
Proposed by Pieter Leclerc
Status: Merged
Merged at revision: not available
Proposed branch: lp://qastaging/~pieterleclerc-deactivatedaccount/gtg/noarchivedlists
Merge into: lp://qastaging/~gtg/gtg/old-trunk
Diff against target: 142 lines (+37/-27)
4 files modified
AUTHORS (+1/-0)
CHANGELOG (+1/-0)
GTG/plugins/rtm_sync/rtmProxy.py (+3/-2)
GTG/plugins/rtm_sync/syncEngine.py (+32/-25)
To merge this branch: bzr merge lp://qastaging/~pieterleclerc-deactivatedaccount/gtg/noarchivedlists
Reviewer Review Type Date Requested Status
Luca Invernizzi (community) Needs Information
Review via email: mp+19165@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Pieter Leclerc (pieterleclerc-deactivatedaccount) wrote :

Fix for https://bugs.launchpad.net/gtg/+bug/520427

Revision history for this message
Pieter Leclerc (pieterleclerc-deactivatedaccount) wrote :

This branch fixes does 3 things:

1) It filters out archived tasks in rtmProxy.py to fix bug #520427.

2) It adds a def __log(self, message) to syncEngine.py because otherwise self.__log("Exception: requested an inexistent task!") would throw an uncatched exception.

3) It catches KeyError's in syncEngine.py because otherwise a KeyError (which might happen for a number of reasons) would throw an uncatched exception.

lp://qastaging/~pieterleclerc-deactivatedaccount/gtg/noarchivedlists updated
596. By Tom Van Braeckel <email address hidden>

Make sure the code is clean

Revision history for this message
Luca Invernizzi (invernizzi) wrote :

Hi Tom,
thanks for your work. I will merge right now all the changes that are not related to bug #520427.

About that bug, I was thinking about how to solve this for the last week (it was aready been reported) . I'm a bit concerned about the approach you took. I find that this way, once a task is marked as done, the link with the related rtm task is broken. It the task is brought back for some reason, it will lead to a task duplication either in rtm or in gtg.
Related to this, I find that the "except KeyError" in syncEngine.py:98 is a bit an hack. There should be no KeyErrors there, and if there are, well, it's better to stop the all the sync (it's in a thread, so it just dies without bringing down gtg). So the principal purpose of the exception is trapping tasks that have been marked as done. Exceptions should be used only for exceptional things, not as very powerful "if".

This is just my opinion, and it can be discussed.

Revision history for this message
Luca Invernizzi (invernizzi) wrote :

Hi Sam, could https://bugs.edge.launchpad.net/gtg/+bug/495758 be an acceptable solution?

review: Needs Information
Revision history for this message
Pieter Leclerc (pieterleclerc-deactivatedaccount) wrote :

> Hi Tom,
> thanks for your work. I will merge right now all the changes that are not
> related to bug #520427.

Thanks !

>
> About that bug, I was thinking about how to solve this for the last week (it
> was aready been reported) . I'm a bit concerned about the approach you took.

Ah, strange - I didn't find that bug report. Could you provide the link ? We should mark them as duplicates then.

> I find that this way, once a task is marked as done, the link with the related
> rtm task is broken. It the task is brought back for some reason, it will lead
> to a task duplication either in rtm or in gtg.

I assume you mean "when the task is marked as archived". If not, then we are talking about different things. This bug is about archived tasks (regardless of whether they have been marked as done).

Your point is still valid for archived tasks as well - in my fix, the link with the RTM task is broken. I'm no GTG expert (this is my first patch), but I think if we want to keep the link with the related RTM task, then we should sync archived tasks just like regular tasks, but hide them throughout the rest of the application.

To do this, we'll need the ability to mark a task (in GTG) as "archived" (or "invisible"), and adjust the code so that these tasks are not shown (by default). Perhaps a configuration option for this would be nice, although I don't think many people would use this.

Do you know if such a possibility is already available somehow ?

> Related to this, I find that the "except KeyError" in syncEngine.py:98 is a
> bit an hack. There should be no KeyErrors there, and if there are, well, it's
> better to stop the all the sync (it's in a thread, so it just dies without
> bringing down gtg). So the principal purpose of the exception is trapping
> tasks that have been marked as done. Exceptions should be used only for
> exceptional things, not as very powerful "if".

Good point, I think you're right. Syncing archived tasks but hiding them would also prevent KeyErrors.

>
> This is just my opinion, and it can be discussed.

Thanks for your valuable feedback !

Revision history for this message
Pieter Leclerc (pieterleclerc-deactivatedaccount) wrote :

> Hi Sam, could https://bugs.edge.launchpad.net/gtg/+bug/495758 be an acceptable
> solution?

Did you mean Tom or was this directed to someone else ?

Anyway, that bug talks about completed tasks while this one here is about archived tasks, so I don't think that would be an acceptable solution...

Revision history for this message
Luca Invernizzi (invernizzi) wrote :

I mean Tom, it was a bit late.
GTG does not have the concept of archived tasks. Does RTM have it?
 Anyway, I'm currently writing a fix for that bug I mentioned, so we could discuss it after it's in trunk (should take no more than an hour).

Revision history for this message
Luca Invernizzi (invernizzi) wrote :

I forgot. You're very welcome in our IRC channel (#gtg on irc.gimp.org) if you want to discuss with some of us.

Revision history for this message
Luca Invernizzi (invernizzi) wrote :

Have a look at the new plugin in trunk and tell me if that would solve this bug. If you come up with a better name for that plugin, please tell me!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'AUTHORS'
2--- AUTHORS 2010-02-08 05:49:45 +0000
3+++ AUTHORS 2010-02-12 11:04:15 +0000
4@@ -53,3 +53,4 @@
5 For 0.3:
6 ----------
7 * Chris Johnston <chrisjohnston@ubuntu.com>
8+* Tom Van Braeckel <tomvanbraeckel@gmail.com>
9
10=== modified file 'CHANGELOG'
11--- CHANGELOG 2010-02-10 10:06:33 +0000
12+++ CHANGELOG 2010-02-12 11:04:15 +0000
13@@ -1,3 +1,4 @@
14+ * Improve RTM plugin (bug #520427): don't sync tasks from archived lists
15 * notification area plugin updated to support appindicate by Luca Invernizzi and Jono Bacon
16 * gtg_new task now supports command switches by Luca Invernizzi
17 * Fix bug #511651, white space around title, by mrk
18
19=== modified file 'GTG/plugins/rtm_sync/rtmProxy.py'
20--- GTG/plugins/rtm_sync/rtmProxy.py 2010-02-01 06:07:20 +0000
21+++ GTG/plugins/rtm_sync/rtmProxy.py 2010-02-12 11:04:15 +0000
22@@ -92,8 +92,10 @@
23 lists_id_list = map(lambda x: x.id, \
24 self.rtm.lists.getList().lists.list)
25
26+ # Download all non-archived tasks in the list with id x
27 def get_list_of_taskseries(x):
28- currentlist = self.rtm.tasks.getList(list_id = x).tasks
29+ currentlist = self.rtm.tasks.getList(list_id = x, \
30+ filter = 'includeArchived:false').tasks
31 if hasattr(currentlist, 'list'):
32 return currentlist.list
33 else:
34@@ -157,7 +159,6 @@
35 else:
36 os.makedirs(dirname)
37
38-
39 def _smartSaveToFile(self, dirname, filename, item, **kwargs):
40 path=dirname+'/'+filename
41 try:
42
43=== modified file 'GTG/plugins/rtm_sync/syncEngine.py'
44--- GTG/plugins/rtm_sync/syncEngine.py 2010-02-11 10:53:11 +0000
45+++ GTG/plugins/rtm_sync/syncEngine.py 2010-02-12 11:04:15 +0000
46@@ -169,32 +169,39 @@
47 "id", \
48 "self")
49 for local_id in updatable_local_ids:
50- taskpair = local_to_taskpair[local_id]
51- local_task = local_id_to_task[local_id]
52- remote_task = remote_id_to_task[taskpair.remote_id]
53- local_was_updated = local_task.modified > \
54+ try:
55+ taskpair = local_to_taskpair[local_id]
56+ local_task = local_id_to_task[local_id]
57+ remote_task = remote_id_to_task[taskpair.remote_id]
58+ local_was_updated = local_task.modified > \
59 taskpair.local_synced_until
60- remote_was_updated = remote_task.modified > \
61+ remote_was_updated = remote_task.modified > \
62 taskpair.remote_synced_until
63
64- if local_was_updated and remote_was_updated:
65- if local_task.modified > remote_task.modified:
66+ if local_was_updated and remote_was_updated:
67+ if local_task.modified > remote_task.modified:
68+ self.update_substatus(_("Updating ") + \
69+ local_task.title)
70+ remote_task.copy(local_task)
71+ else:
72+ #If the update time is the same one, we have to
73+ # arbitrary decide which gets copied
74+ self.update_substatus(_("Updating ") + \
75+ remote_task.title)
76+ local_task.copy(remote_task)
77+ elif local_was_updated:
78 self.update_substatus(_("Updating ") + local_task.title)
79 remote_task.copy(local_task)
80- else:
81- #If the update time is the same one, we have to
82- # arbitrary decide which gets copied
83+ elif remote_was_updated:
84 self.update_substatus(_("Updating ") + remote_task.title)
85 local_task.copy(remote_task)
86- elif local_was_updated:
87- self.update_substatus(_("Updating ") + local_task.title)
88- remote_task.copy(local_task)
89- elif remote_was_updated:
90- self.update_substatus(_("Updating ") + remote_task.title)
91- local_task.copy(remote_task)
92-
93- taskpair.remote_synced_until = remote_task.modified
94- taskpair.local_synced_until = local_task.modified
95+
96+ taskpair.remote_synced_until = remote_task.modified
97+ taskpair.local_synced_until = local_task.modified
98+ except KeyError:
99+ self.__log("Warning: failed to update task, possibly because \
100+ a previous version had sync'ed archived tasks.")
101+
102
103 #Lastly, save the list of known links
104 self.update_status(_("Saving current state.."))
105@@ -204,8 +211,7 @@
106 "taskpairs", \
107 self.taskpairs,
108 xdg_data_home)
109- self.close_gui(_("Synchronization completed."))
110-
111+ self.close_gui(_("Synchronization completed."))
112
113 def _append_to_taskpairs(self, local_tasks, remote_tasks):
114 for local, remote in zip(local_tasks, remote_tasks):
115@@ -218,13 +224,11 @@
116 id_to_task = self._list_to_dict(task_list, "id", "self")
117 result=[]
118 for id in id_list:
119- if id_to_task.has_key(id):
120+ if id in id_to_task:
121 result.append(id_to_task[id])
122 else:
123 self.__log("Exception: requested an inexistent task!")
124 return result
125-
126-
127
128 def _process_new_tasks(self, new_ids, all_tasks, proxy):
129 new_tasks = self._task_ids_to_tasks(new_ids, all_tasks)
130@@ -261,8 +265,11 @@
131 local_task = local_title_to_task[title],
132 remote_task = remote_title_to_task[title]))
133
134+ def __log(self, message):
135+ if self.logger:
136+ self.logger.debug(message)
137
138-## GUI
139+## GUI
140 def close_gui(self, msg):
141 self.update_status(msg)
142 self.update_progressbar(1.0)

Subscribers

People subscribed via source and target branches

to status/vote changes: