Merge lp:~mterry/aptdaemon/progress-tweaks into lp:aptdaemon

[Michael Terry]

This branch implements some of the progress dialog changes from https://wiki.ubuntu.com/SoftwareUpdates

Specifically:
1) Drops the in-dialog icon
2) Updates a couple of the header labels' wording
3) Moves the cancel button to the right of the progress bar
4) Allows minimizing the dialog
5) Adds an API call to ask not to update the title bar (the consumer can then set it to something custom)
6) Adds an API call to ask not to show the details expander

[Sebastian Heinlein (glatzor)]

Am Dienstag, den 05.06.2012, 17:29 +0000 schrieb Michael Terry:
> Michael Terry has proposed merging lp:~mterry/aptdaemon/progress-tweaks into lp:aptdaemon.
>
> Requested reviews:
> Aptdaemon Developers (aptdaemon-developers)
>
> For more details, see:
> https://code.launchpad.net/~mterry/aptdaemon/progress-tweaks/+merge/108789
>
> This branch implements some of the progress dialog changes from https://wiki.ubuntu.com/SoftwareUpdates
>
> Specifically:
> 1) Drops the in-dialog icon

The cute little animation... :)

> 2) Updates a couple of the header labels' wording

Be careful: ROLE_COMMIT_PACKAGES can be an installation, removal, update
or downgrade of packages. Even at the same time. So you cannot reduce
the name to installing updates only.

If you only want to update packages you should use the update packages
call in update-manager.

Why do you want to add the three points? At first this should be
consistent with all other enums. Secondly it should be consistent with
the other apps from the GNOME desktop.

If you want to set a custom label we should better add support for this
to the dialog: Perhaps adding a message_format or label parameter to
__init__()?

> 5) Adds an API call to ask not to update the title bar (the consumer can then set it to something custom)

I would prefer if the title would be set to the role automatically if it
is empty instead of introducing another setter.

if not self.get_title():
    self.set_title(role)

The role of a transaction cannot be changed and so will always be the
same.

Or is this too much magic?

> 6) Adds an API call to ask not to show the details expander

It would be nice to have a hidden option for update-manager to allow
showing the details (gnome-tweaks?) if you plan to remove them by
default. It is quite popular by users to watch the downloads - it seems
to give them some feeling of control. Alternatively the terminal can be
hidden by the terminal parameter separately.

Thanks,

Sebastian

[lp:~mterry/aptdaemon/progress-tweaks]

834. By Michael Terry on 2012-06-06

changes from merge review

[Michael Terry (mterry)]

Hello, Sebastian! Thanks for the review.

> Be careful: ROLE_COMMIT_PACKAGES can be an installation, removal, update
> or downgrade of packages

Ah... Hmm... It looks like AptClient.install_packages() can do both upgrades and installs? That can work for update-manager.

The ellipsis is common for such in-progress labels. I added it because it was in mpt's spec. But it's used throughout GNOME too. As for the consistency with other roles, I agree.

> The role of a transaction cannot be changed and so will always be the
> same.

Ah! I assumed it could, because we connect to the "role-changed" signal on the transaction. If that signal only ever fires zero or one time (I'm assuming the signal is for transactions where the role isn't set immediately...), then your check-for-empty-title solution seems fine.

> It would be nice to have a hidden option for update-manager to allow
> showing the details

I guess that's a call for mpt, but that can be done update-manager side. I think the API I added here would be suitable regardless.

I've updated the branch to use your empty-title fix, to add ellipses to all the roles, to revert the COMMIT role back to "Applying changes", and to change the INSTALL role to "Installing updates". Could you review again?

[Michael Terry (mterry)]

(As for adding an API for an explicit custom label, I don't believe we *need* that yet, but it wouldn't hurt, and might let update-manager get the ellipsis if you still want to reject them.)

[Michael Terry (mterry)]

Actually, "Installing updates" probably doesn't make much sense in the general sense of installing new packages. So maybe that change shouldn't be made and I should just add a custom way to set the label... I'll work on that.

[lp:~mterry/aptdaemon/progress-tweaks]

835. By Michael Terry on 2012-06-06

revert changes to Role labels, add ability to specify a custom label for the progress dialog; and add back a line that accidentally got deleted that makes hiding the details view actually work

[Michael Terry (mterry)]

OK, try now. I added a label=None keyword argument to the __init__ function. If set, the role won't be used for the main label.

I also reverted the rest of the role label changes since they weren't needed anymore.

[Sebastian Heinlein (glatzor)]

Am Mittwoch, den 06.06.2012, 18:58 +0000 schrieb Michael Terry:
> OK, try now. I added a label=None keyword argument to the __init__ function. If set, the role won't be used for the main label.
>
> I also reverted the rest of the role label changes since they weren't needed anymore.

I will have a look at it on Friday. By the way in which time zone do you
live? The US?

[Michael Terry (mterry)]

Sure, no rush. I live in EST (Boston).

[Matthew Paul Thomas (mpt)]

In the long term, I think Update Manager should use its own progress UI rather than Aptdaemon's progress window. In <https://wiki.ubuntu.com/SoftwareUpdates#Installing> I show the initial prompt morphing into the progress window, when later morphs into the report of success or failure; that's probably difficult as long as they're separate executables. More importantly, Aptdaemon's progress window probably should be modal to its parent window, so that -- for example -- a media player isn't confused by you doing something else in its window while it is blocked waiting for a codec to install. The Software Updater progress window, meanwhile, shouldn't be modal to anything.

If that's true, then this --

- functions = Gdk.WMFunction.MOVE|Gdk.WMFunction.RESIZE
+ functions = Gdk.WMFunction.MOVE|Gdk.WMFunction.RESIZE|Gdk.WMFunction.MINIMIZE

-- is not a good idea. You shouldn't be able to minimize a dialog independently of its parent window.

+ # Setup the label, progressbar, and cancel button

Grammar nit: "Setup" is a noun. The verb is "Set up". (I mention this because it's a common UI mistake too.)

+ self.label_role.set_markup("<big><b>%s</b></big>" % self.custom_label)

+ self.label_role.set_markup("<big><b>%s</b></big>" % role)

This is a progress window, not an alert, so the text shouldn't be any larger or bolder than normal.

Thanks!

review: Needs Fixing (design)

[Michael Terry (mterry)]

> -- is not a good idea. You shouldn't be able to minimize a dialog independently of its parent window.

I believe the WM is responsible for handling such cases. For example, in Unity with this patch, if the aptdaemon window is alone, minimize is shown. When it is modal, minimize is not. So I think that line is safe.

> Grammar nit: "Setup" is a noun. The verb is "Set up". (I mention this because it's a common UI mistake too.)

True. That nit was in the original code, but I suppose I could have fixed with my patch.

> This is a progress window, not an alert, so the text shouldn't be any larger or bolder than normal.

Again, this was in the original code. This would be another design difference from mpt's spec, so if you are looking to change aptdaemon to no longer have bold & large labels there, I'd have to either hack around your changes in update-manager or roll my own progress dialog sooner rather than later.

[Michael Terry (mterry)]

Hah, sorry, didn't realize you had replied, mpt. I thought it was Sebastian still. So let me ask you to clarify then. In your mockups, it looks like the role label is larger than the text below the progress bar.

[Michael Terry (mterry)]

But you are right that the morphing will be tough without us owning the widgets (considering that it will likely involve just one GtkWindow and just swapping out the contents). So maybe this branch isn't super important and I can work on copying the widget into update-manager's source (it's not that much code).

This branch is still useful though to achieve a consistent look.

[Sebastian Heinlein (glatzor)]

Am Montag, den 11.06.2012, 15:30 +0000 schrieb Michael Terry:
> So maybe this branch isn't super important and I can work on copying the widget into update-manager's source (it's not that much code).

So please don't copy any code of the widgets into different projects.
The gtk(3)widgets modules provides a set of widgets which can used to
monitor and control an aptdaemon transaction and can easily be re-used:
Status label, progress bar, cancel dialog or details expander. The
AptProgressDialog was just a kind of generic show-case dialog to provide
a fast starting point for aptdaemon clients.

> This branch is still useful though to achieve a consistent look.

The dialog is used by sessioninstaller, too. But we could think about
moving sessiontionstaller to a morphing approach (notebook with hidden
tabs or assistant).

Cheers,

Sebastian

[Sebastian Heinlein (glatzor)]

Am Montag, den 11.06.2012, 14:57 +0000 schrieb Matthew Paul Thomas:
> Review: Needs Fixing design
>
> In the long term, I think Update Manager should use its own progress UI rather than Aptdaemon's progress window. In <https://wiki.ubuntu.com/SoftwareUpdates#Installing> I show the initial prompt morphing into the progress window, when later morphs into the report of success or failure; that's probably difficult as long as they're separate executables. More importantly, Aptdaemon's progress window probably should be modal to its parent window, so that -- for example -- a media player isn't confused by you doing something else in its window while it is blocked waiting for a codec to install. The Software Updater progress window, meanwhile, shouldn't be modal to anything.

A small technical correction: The aptdaemon dialog is part of the
update-manager process. The dialog is created by update-manager.

But sessioninstaller (the codec installer) is a different story. The
codec requesting application calls a D-Bus (PackageKit) method and so
starts the codec installer on demand. The D-Bus call also includes a
window id to which sessioninstaller should be transient for. So if this
isn't the case you should fill a bug.

> Grammar nit: "Setup" is a noun. The verb is "Set up". (I mention this because it's a common UI mistake too.)

This is also a common mistake of me. :)

> + self.label_role.set_markup("<big><b>%s</b></big>" % self.custom_label)
>
> + self.label_role.set_markup("<big><b>%s</b></big>" % role)
>
> This is a progress window, not an alert, so the text shouldn't be any larger or bolder than normal.

Good point, but it seems to introduce a strange weighting. It makes the
difference to the status label harder to spot. I attached some
screenshots to this mail.

[Michael Terry (mterry)]

> So please don't copy any code of the widgets into different projects.
> The gtk(3)widgets modules provides a set of widgets which can used to

I wasn't planning on copying the widgets like the progress bar or details expander. Just the dialog that holds them together.

[Matthew Paul Thomas (mpt)]

> In your mockups, it looks like the role label is larger than the text below the progress bar.

> It makes the difference to the status label harder to spot. I attached some screenshots to this mail.

I think the role label should be normal size, while the text below the progress bar should be a caption <small>. I'm sorry my sketches weren't quite high-resolution enough to show this. :-)

Apart from that, the screenshots Sebastian forwarded me look good -- both for the aptdaemon standalone progress dialog, and as a jumping-off point for Software Updater's variation of the dialog.

Unmerged revisions

835. By Michael Terry on 2012-06-06

revert changes to Role labels, add ability to specify a custom label for the progress dialog; and add back a line that accidentally got deleted that makes hiding the details view actually work

834. By Michael Terry on 2012-06-06

changes from merge review

833. By Michael Terry on 2012-06-05

and same changes for gtk2 widgets too

832. By Michael Terry on 2012-06-05

UI tweaks to the apt progress dialog to match mpt's spec for the Software Updater

=== modified file 'aptdaemon/gtk3widgets.py'
--- aptdaemon/gtk3widgets.py	2012-05-12 02:09:10 +0000
+++ aptdaemon/gtk3widgets.py	2012-06-06 18:57:18 +0000
@@ -540,47 +540,50 @@
                                  GObject.TYPE_NONE, ())}
 
     def __init__(self, transaction=None, parent=None, terminal=True,
-                 debconf=True):
+                 debconf=True, label=None):
         Gtk.Dialog.__init__(self, parent=parent)
         self._expanded_size = None
         self.debconf = debconf
+        self.custom_label = label
         # Setup the dialog
         self.set_border_width(6)
         self.set_resizable(False)
         self.get_content_area().set_spacing(6)
-        # Setup the cancel button
-        self.button_cancel = AptCancelButton(transaction)
-        self.get_action_area().pack_start(self.button_cancel, False, False, 0)
-        # Setup the status icon, label and progressbar
-        hbox = Gtk.HBox()
-        hbox.set_spacing(12)
-        hbox.set_border_width(6)
-        self.icon = AptRoleIcon()
-        hbox.pack_start(self.icon, False, True, 0)
+        # Setup the label, progressbar, and cancel button
         vbox = Gtk.VBox()
         vbox.set_spacing(12)
+        vbox.set_border_width(6)
         self.label_role = Gtk.Label()
         self.label_role.set_alignment(0, 0)
+        if self.custom_label:
+            self.label_role.set_markup("<big><b>%s</b></big>" % self.custom_label)
         vbox.pack_start(self.label_role, False, True, 0)
+        hbox = Gtk.HBox()
+        hbox.set_spacing(12)
         vbox_progress = Gtk.VBox()
         vbox_progress.set_spacing(6)
+        align = Gtk.Alignment()
+        align.set(0.0, 0.5, 1.0, 0.5)
         self.progress = AptProgressBar()
-        vbox_progress.pack_start(self.progress, False, True, 0)
+        align.add(self.progress)
+        hbox.pack_start(align, True, True, 0)
+        self.button_cancel = AptCancelButton(transaction)
+        hbox.pack_start(self.button_cancel, False, False, 0)
+        vbox_progress.pack_start(hbox, False, True, 0)
         self.label = AptStatusLabel()
         self.label._on_status_changed(None, STATUS_WAITING)
         vbox_progress.pack_start(self.label, False, True, 0)
         vbox.pack_start(vbox_progress, False, True, 0)
-        hbox.pack_start(vbox, True, True, 0)
         self.expander = AptDetailsExpander(terminal=terminal)
         self.expander.connect("notify::expanded", self._on_expanded)
         vbox.pack_start(self.expander, True, True, 0)
-        self.get_content_area().pack_start(hbox, True, True, 0)
+        self.get_content_area().pack_start(vbox, True, True, 0)
         self._transaction = None
         self._signals = []
         self.set_title("")
         self.realize()
         self.progress.set_size_request(350, -1)
-        functions = Gdk.WMFunction.MOVE|Gdk.WMFunction.RESIZE
+        functions = Gdk.WMFunction.MOVE|Gdk.WMFunction.RESIZE|Gdk.WMFunction.MINIMIZE
         try:
             self.get_window().set_functions(functions)
         except TypeError:
@@ -685,8 +688,10 @@
     def _on_role_changed(self, transaction, role_enum):
         """Show the role of the transaction in the dialog interface"""
         role = get_role_localised_present_from_enum(role_enum)
-        self.set_title(role)
-        self.label_role.set_markup("<big><b>%s</b></big>" % role)
+        if not self.get_title():
+            self.set_title(role)
+        if self.custom_label is None:
+            self.label_role.set_markup("<big><b>%s</b></big>" % role)
 
     def set_transaction(self, transaction):
         """Connect the dialog to the given aptdaemon transaction"""
@@ -703,7 +708,6 @@
                              self._on_config_file_conflict))
         self._on_role_changed(transaction, transaction.role)
         self.progress.set_transaction(transaction)
-        self.icon.set_transaction(transaction)
         self.label.set_transaction(transaction)
         self.expander.set_transaction(transaction)
         self._transaction = transaction
@@ -735,6 +739,10 @@
             err_dia.hide()
         self.emit("finished")
 
+    def set_show_details(self, show_details):
+        self.expander.set_no_show_all(not show_details)
+        self.expander.set_visible(show_details)
+
 
 class _ExpandableDialog(Gtk.Dialog):
 

=== modified file 'aptdaemon/gtkwidgets.py'
--- aptdaemon/gtkwidgets.py	2012-05-08 18:50:57 +0000
+++ aptdaemon/gtkwidgets.py	2012-06-06 18:57:18 +0000
@@ -535,48 +535,51 @@
                                  gobject.TYPE_NONE, ())}
 
     def __init__(self, transaction=None, parent=None, terminal=True,
-                 debconf=True):
+                 debconf=True, label=None):
         gtk.Dialog.__init__(self, buttons=None, parent=parent)
         self._expanded_size = None
         self.debconf = debconf
+        self.custom_label = label
         # Setup the dialog
         self.set_border_width(6)
         self.set_has_separator(False)
         self.set_resizable(False)
         self.vbox.set_spacing(6)
-        # Setup the cancel button
-        self.button_cancel = AptCancelButton(transaction)
-        self.action_area.pack_start(self.button_cancel, False, False, 0)
-        # Setup the status icon, label and progressbar
-        hbox = gtk.HBox()
-        hbox.set_spacing(12)
-        hbox.set_border_width(6)
-        self.icon = AptRoleIcon()
-        hbox.pack_start(self.icon, False, True, 0)
+        # Setup the label, progressbar, and cancel button
         vbox = gtk.VBox()
         vbox.set_spacing(12)
+        vbox.set_border_width(6)
         self.label_role = gtk.Label()
         self.label_role.set_alignment(0, 0)
+        if self.custom_label:
+            self.label_role.set_markup("<big><b>%s</b></big>" % self.custom_label)
         vbox.pack_start(self.label_role, False, True, 0)
         vbox_progress = gtk.VBox()
+        hbox = gtk.HBox()
+        hbox.set_spacing(12)
         vbox_progress.set_spacing(6)
+        align = gtk.Alignment()
+        align.set(0.0, 0.5, 1.0, 0.5)
         self.progress = AptProgressBar()
-        vbox_progress.pack_start(self.progress, False, True, 0)
+        align.add(self.progress)
+        hbox.pack_start(align, True, True, 0)
+        self.button_cancel = AptCancelButton(transaction)
+        hbox.pack_start(self.button_cancel, False, False, 0)
+        vbox_progress.pack_start(hbox, False, True, 0)
         self.label = AptStatusLabel()
         self.label._on_status_changed(None, STATUS_WAITING)
         vbox_progress.pack_start(self.label, False, True, 0)
         vbox.pack_start(vbox_progress, False, True, 0)
-        hbox.pack_start(vbox, True, True, 0)
         self.expander = AptDetailsExpander(terminal=terminal)
         self.expander.connect("notify::expanded", self._on_expanded)
         vbox.pack_start(self.expander, True, True, 0)
-        self.vbox.pack_start(hbox, True, True, 0)
+        self.vbox.pack_start(vbox, True, True, 0)
         self._transaction = None
         self._signals = []
         self.set_title("")
         self.realize()
         self.progress.set_size_request(350, -1)
-        self.window.set_functions(gtk.gdk.FUNC_MOVE|gtk.gdk.FUNC_RESIZE)
+        self.window.set_functions(gtk.gdk.FUNC_MOVE|gtk.gdk.FUNC_RESIZE|gtk.gdk.FUNC_MINIMIZE)
         if transaction != None:
             self.set_transaction(transaction)
         # catch ESC and behave as if cancel was clicked
@@ -640,8 +643,10 @@
     def _on_role_changed(self, transaction, role_enum):
         """Show the role of the transaction in the dialog interface"""
         role = get_role_localised_present_from_enum(role_enum)
-        self.set_title(role)
-        self.label_role.set_markup("<big><b>%s</b></big>" % role)
+        if not self.get_title():
+            self.set_title(role)
+        if self.custom_label is None:
+            self.label_role.set_markup("<big><b>%s</b></big>" % role)
 
     def set_transaction(self, transaction):
         """Connect the dialog to the given aptdaemon transaction"""
@@ -656,7 +661,6 @@
                              self._on_config_file_conflict))
         self._on_role_changed(transaction, transaction.role)
         self.progress.set_transaction(transaction)
-        self.icon.set_transaction(transaction)
         self.label.set_transaction(transaction)
         self.expander.set_transaction(transaction)
         self._transaction = transaction
@@ -688,6 +692,10 @@
             err_dia.hide()
         self.emit("finished")
 
+    def set_show_details(self, show_details):
+        self.expander.set_no_show_all(not show_details)
+        self.expander.set_visible(show_details)
+
 
 class _ExpandableDialog(gtk.Dialog):