task.add_tag behavior unclear

[Luca Invernizzi]

the add_tag method adds a tag to the task without adding the corresponding text inside the tag description. That behavior is provided by task's insert_tag, but the distinction isn't clear until one looks at the code or gets a bug.
This caused bug #460751, and also a bug in rtm plugin (fixed, not reported).
Maybe we should change the signature of that function?

[Lionel Dricot (ploum-deactivatedaccount)]

I'm 100% in favour of making things easier. The only concern is to track the usage of the function everywhere to be sure that a signature change will not affect other functions.

Maybe is there a reason for such a behaviour? If not, I agree with your proposition.

[Luca Invernizzi (invernizzi)]

bug #498572 adds Kevin to the club of the "add_tag() betrayed me!".
Paulo (in the helloworld plugin) and I welcome you, kevin.

[Luca Invernizzi (invernizzi)]

check what kevin has done in bug #498572

[Lionel Dricot (ploum-deactivatedaccount)]

just to be sure : what we want here is that the Task.add_tag() method add the tag to the text of the task if that tag is not present. Is that right?

I see two possible approaches :

1) We modify the text of a task when adding a tag.

2) When we open a task with the editor, we add tags that are not in the text.

I don't know which one would be more appropriate. What do you think?

[Lionel Dricot (ploum-deactivatedaccount)]

I remove myself from the assignee if Kevin is working on it.

[Kevin Mehall (kevin-mehall)]

I wrote a fix using approach 1 in http://bazaar.launchpad.net/~gtg/gtg/drag-and-drop/revision/485

It's a separate insert_tag() method, so add_tag() can still be used for cases where the '@tag_name' text does not have to be inserted, such as if the user adds a tag by typing in the editor window.

[Lionel Dricot (ploum-deactivatedaccount)]

nice job. Do you think it worth renaming your function add_tag and renaming the old one to something like tag_added ?

[Kevin Mehall (kevin-mehall)]

Lionel> I renamed add_tag to tag_added.

Notes on call sites for the old add_tag:

./GTG/plugins/bugzilla/bugzilla.py:64: task.add_tag('@%s' % tag)
   -> "Changed" to add_tag because I think it fixes an existing bug

./GTG/plugins/rtm_sync/generic_task.py:256: map(lambda tag: self.task.add_tag('@'+tag), tags)
   -> left alone (tag_added) for now. take a look and see if this could be simplified with new add_tag

./GTG/core/dbuswrapper.py: for tag in tags: nt.add_tag(tag)
   -> used tags keyword parameter of req.new_task instead

./GTG/core/dbuswrapper.py:125: for tag in task_data["tags"]: task.add_tag(tag)
   -> should be add_tag so other apps don't have to handle this

./GTG/core/plugins/api.py:425: self.__requester.get_task(tid).add_tag("@" + tag)
./GTG/core/plugins/api.py:427: self.task.add_tag("@" + tag)
    -> "Changed" to add_tag so plugins don't need to handle this. Hopefully no breakage.

./GTG/core/requester.py:89: task.add_tag(t.get_name())
    -> Use tag_added

./GTG/core/task.py:320: task.add_tag(t.get_name())
    -> Use tag_added (under if task.can_be_deleted, so editor handles this case)

./GTG/core/task.py:512: child.add_tag(tagname)
    -> Use tag_added (this behavior probably should be discussed --
       why do we only add tags if the child has not been modified)

./GTG/core/task.py:517: if self.add_tag(tagname):
    -> tag_added (inside implementation of add_tag)

./GTG/tools/taskxml.py:61: for tag in cur_tags: cur_task.add_tag(tag)
    -> tag_added, since loaded text should already contain tags