Merge lp:~kiddo/gtg/pretty-delete-confirmation into lp:~gtg/gtg/old-trunk

[Jeff Fortin Tam]

[Jeff Fortin Tam (kiddo)]

Fixed the various usability issues I saw with the current delete confirmation dialog.

[Bertrand Rousseau (bertrand-rousseau)]

Thanks for your patch! I have 2 small remarks:

1 - in browser.py, around line 1364, could you rename variable self.tasks_to_delete as self.del_tasks_cnt (or similar), just to make clear that it is not a list of tasks but an integer. It looks a bit confusing.

2 - could update CHANGELOG and AUTHORS by mentioning your changes and listing you as a contributor for the current version?

Thanks!

review: Needs Fixing

[Lionel Dricot (ploum-deactivatedaccount)]

I don't agree. I don't understand why we should remove the title of removed tags. We should at least list the first one because removing a task also remove the subtasks and, without some titles, you might click OK without checking.

[Bertrand Rousseau (bertrand-rousseau)]

Hmm, I didn't see that change. You're right, it should stay, but I also think it should also be changed since displaying all the deleted tasks in the dialog text also clutters it. We probably should adopt a behavior similar to gedit's when you close it with several unsaved documents: having a small treeview inside the the dialog when several tasks are going to be removed. By using this, we allow scrolling and we maintain dialog sizes to sane values. But that's another discussion that requires to open a bug. As far as this patch is concerned, this removal forces me to disapprove the patch.

review: Disapprove

[Jeff Fortin Tam (kiddo)]

I did not see this as a problem, as it 1) mentions that there are
multiple tasks selected 2) the user just made the *explicit* action of
selecting them.

Since the user just took the time to manually select which tasks to
delete, I can pretty much guarantee you that he/she is supposed to know
what he just selected. Bringing the list up to his face a second time
will *not* make him/her double-check, it will just annoy the user. Why
would the user need to see this stuff twice? Does the gnome trash show
you the "list" of files you are permanently deleting when you empty it?
Nope :)

The reasons I see for the existence of this dialog are:
1) prevent accidents resulting from hitting the Delete key on the
keyboard, or the wrong toolbar button (since there's no undo)
2) warn the user that there is no undo
3) warn the user that there is/isn't more than one item selected

It's not like gedit's unsaved document feature. There's no concept of
save vs unsaved here. It's not a "did you forget?", it's a "warning, was
it you that triggered this action or was it your cat walking on your
keyboard?"

The user wants to friggin' delete them :) keep it simple!

[Bertrand Rousseau (bertrand-rousseau)]

It could also happen that you choose to delete a parent thinking it's
ok, then you realise that there's a chid task you still need to keep.
It happened to me once, I was relieved having this list remembering me
this task before I deleted it.

On Thu, Dec 17, 2009 at 9:18 PM, Jean-François Fortin Tam
<email address hidden> wrote:
> I did not see this as a problem, as it 1) mentions that there are
> multiple tasks selected 2) the user just made the *explicit* action of
> selecting them.
>
> Since the user just took the time to manually select which tasks to
> delete, I can pretty much guarantee you that he/she is supposed to know
> what he just selected. Bringing the list up to his face a second time
> will *not* make him/her double-check, it will just annoy the user. Why
> would the user need to see this stuff twice? Does the gnome trash show
> you the "list" of files you are permanently deleting when you empty it?
> Nope :)
>
> The reasons I see for the existence of this dialog are:
> 1) prevent accidents resulting from hitting the Delete key on the
> keyboard, or the wrong toolbar button (since there's no undo)
> 2) warn the user that there is no undo
> 3) warn the user that there is/isn't more than one item selected
>
> It's not like gedit's unsaved document feature. There's no concept of
> save vs unsaved here. It's not a "did you forget?", it's a "warning, was
> it you that triggered this action or was it your cat walking on your
> keyboard?"
>
> The user wants to friggin' delete them :) keep it simple!
>
> --
> https://code.launchpad.net/~kiddo/gtg/pretty-delete-confirmation/+merge/16294
> You are reviewing the proposed merge of lp:~kiddo/gtg/pretty-delete-confirmation into lp:gtg.
>

--
Bertrand Rousseau

[Jeff Fortin Tam (kiddo)]

Oooh, I see your point now. Well perhaps what could be done is to show
the list if there's only a parent selected, with a third message along
the lines of "hey, it's a parent task, all the following subtasks will
also be deleted: ...". But then this would be beyond my skills and I
wouldn't be able to implement such a listview as you folks suggested.

Other commits in that branch (such as spacing fixes and button label
fixes) might also be of use though.

[Bertrand Rousseau (bertrand-rousseau)]

We might consider having two different dialogs, yes. Maybe one when
deleting a parent task having uncompleted child tasks (with treeview),
and another for the other cases similar to yours. I'd like to hear
from other people what they think about that.

On Thu, Dec 17, 2009 at 9:30 PM, Jean-François Fortin Tam
<email address hidden> wrote:
> Oooh, I see your point now. Well perhaps what could be done is to show
> the list if there's only a parent selected, with a third message along
> the lines of "hey, it's a parent task, all the following subtasks will
> also be deleted: ...". But then this would be beyond my skills and I
> wouldn't be able to implement such a listview as you folks suggested.
>
> Other commits in that branch (such as spacing fixes and button label
> fixes) might also be of use though.
>
> --
> https://code.launchpad.net/~kiddo/gtg/pretty-delete-confirmation/+merge/16294
> You are reviewing the proposed merge of lp:~kiddo/gtg/pretty-delete-confirmation into lp:gtg.
>

--
Bertrand Rousseau

[Lionel Dricot (ploum-deactivatedaccount)]

What about :

You asked to remove X tasks but this will result in also deleting Y subtasks listed here:
-
-
-

(btw, I like Gedit approach)

[Luca Invernizzi (invernizzi)]

I agree that we could have a better delete-dialog, but I don't think that this is the right way.
Since no consensus has been reached, I'll close this bug request for now, as two different discussions on the same matter are ongoing here and in bug #497798 (please move the discussion there).

review: Disapprove

Unmerged revisions

484. By Jeff Fortin Tam on 2009-12-17

HIG: "Between labels and associated components, leave 12 horizontal pixels."

483. By Jeff Fortin Tam on 2009-12-17

Cleanup button labels, this also makes them fit better with singular/plural

482. By Jeff Fortin Tam on 2009-12-17

Create a set of singular/plural labels that simply mention the number of tasks that may be deleted

481. By Jeff Fortin Tam on 2009-12-17

Tweak the spacing values a little bit

=== modified file 'GTG/taskbrowser/browser.py'
--- GTG/taskbrowser/browser.py	2009-12-17 11:09:37 +0000
+++ GTG/taskbrowser/browser.py	2009-12-17 16:24:15 +0000
@@ -1364,15 +1364,18 @@
         else:
             self.tids_todelete = [tid]
         #We must at least have something to delete !
-        if len(self.tids_todelete) > 0:
+        self.tasks_to_delete = len(self.tids_todelete)
+        if self.tasks_to_delete > 0:
             label = self.builder.get_object("label1") 
             label_text = label.get_text()
-            label_text = label_text[0:label_text.find(":") + 1]
             # I find the tasks that are going to be deleted
             titles_list = [self.req.get_task(tid).get_title() \
                               for tid in self.tids_todelete]
             titles = reduce (lambda x, y: x + "\n - " + y, titles_list)
-            label.set_text("%s %s" % (label_text, titles))
+            if self.tasks_to_delete == 1:
+                label.set_text(_("Do you want to permanently delete this task? This operation cannot be undone."))
+            else:
+                label.set_text(_("Do you want to permanently delete those %i tasks? This operation cannot be undone.") % (self.tasks_to_delete))
             delete_dialog = self.builder.get_object("confirm_delete")
             delete_dialog.run()
             delete_dialog.hide()

=== modified file 'GTG/taskbrowser/taskbrowser.glade'
--- GTG/taskbrowser/taskbrowser.glade	2009-12-17 07:57:00 +0000
+++ GTG/taskbrowser/taskbrowser.glade	2009-12-17 16:24:15 +0000
@@ -621,7 +621,7 @@
       <object class="GtkVBox" id="dialog-vbox1">
         <property name="visible">True</property>
         <property name="orientation">vertical</property>
-        <property name="spacing">2</property>
+        <property name="spacing">12</property>
         <child>
           <object class="GtkHBox" id="hbox3">
             <property name="visible">True</property>
@@ -633,27 +633,25 @@
               </object>
               <packing>
                 <property name="fill">False</property>
-                <property name="padding">16</property>
+                <property name="padding">12</property>
                 <property name="position">0</property>
               </packing>
             </child>
             <child>
               <object class="GtkLabel" id="label1">
                 <property name="visible">True</property>
-                <property name="label" translatable="yes">&lt;span weight="bold" size="large"&gt;Are you sure you want delete these tasks?&lt;/span&gt;
-
-Deleting a task cannot be undone, and will delete the following tasks: </property>
+                <property name="label" translatable="yes">Do you want to permanently delete those %i tasks? This operation cannot be undone. (note: this text label is overwritten in the code)</property>
                 <property name="use_markup">True</property>
                 <property name="wrap">True</property>
               </object>
               <packing>
-                <property name="padding">16</property>
+                <property name="padding">12</property>
                 <property name="position">1</property>
               </packing>
             </child>
           </object>
           <packing>
-            <property name="padding">16</property>
+            <property name="padding">12</property>
             <property name="position">1</property>
           </packing>
         </child>
@@ -683,7 +681,7 @@
                       <object class="GtkLabel" id="label2">
                         <property name="visible">True</property>
                         <property name="xalign">0</property>
-                        <property name="label" translatable="yes">_Keep selected tasks</property>
+                        <property name="label" translatable="yes">_Keep</property>
                         <property name="use_underline">True</property>
                       </object>
                       <packing>
@@ -721,7 +719,7 @@
                     <child>
                       <object class="GtkLabel" id="label3">
                         <property name="visible">True</property>
-                        <property name="label" translatable="yes">Permanently _remove tasks</property>
+                        <property name="label" translatable="yes">Permanently _remove</property>
                         <property name="use_underline">True</property>
                       </object>
                       <packing>