Merge lp:~gary-lasker/software-center/recommender-panel-visual-tweaks into lp:software-center

[Gary Lasker]

* lp:~gary-lasker/software-center/recommender-panel-visual-tweaks:
   - add support for two sizes of spinner to the spinner widget and use
     the smaller one for the lobby/category screen panels

Just a small tweak in the recommender panel UI before UI freeze. Adds support in the spinner widget for choosing between two sizes of spinner, and switch to use the smaller version in the lobby/category screen recommender panels as it looks much better there. The large spinner is still used for the full views as before.

Thanks!

[Michael Vogt (mvo)]

Thanks for this branch. I like the size change, it does look nicer IMO with the smaller size.

The code is nice, I would have two suggestions:
- raise a ValueError is spinner_size is not in (Spinner.LARGE, SpinnerView.SMALL) as a user of the widget may pass "24" or "48" to it assuming size is in pixel
- add a small test as test_spinner.py has really good coverage already, should be as simple as:
    def test_spinner_notebook_size(self):
        notebook = spinner.SpinnerNotebook(self.content, "msg", spinner.SpinnerView.LARGE)
        self.assertEqual(notebook.spinner_view.spinner.get_size_request(), (48, 48))
plus a test that the exception is raised.

If you want to go wild, you can do it in enums.py as:

class SpinnerSize:
  LARGE = 48
  SMALL = 24

this way if the size ever changes or we need MEDIUM its trivial to add in enums.py. But thats not really
needed, just a thought I had while looking at the diff.

[lp:~gary-lasker/software-center/recommender-panel-visual-tweaks]

3128. By Gary Lasker on 2012-08-28

pep8 fix

3129. By Gary Lasker on 2012-08-28

add a ValueError check for the spinner_size attribute, add test to test_spinner.py to test a small spinner, a large spinner (default), and finally one for the ValueError case

[Gary Lasker (gary-lasker)]

Hi Michael, thanks for your review and your suggestions! I added the check for spinner_size and raise a ValueError as you suggested, and I've added a test case to test_spinner.py that checks 1. the small spinner case, 2. the large spinner case (as default) and finally 3. a ValueError if a disallowed value for spinner_size is passed in.

I initially considered adding the actual sizes to enums.py as you suggested, or even letting them be specified directly in pixels, but I decided that I really don't forsee us needing any more than these two sizes - the large one for full views and the smaller one for subpanels. That, plus given that we really don't want to make it easy to create arbitrarily-sized spinner panels, I thought it best to keep it to the two useful choices and just embed the pixel details inside the class.

Thanks again!

[Michael Vogt (mvo)]

On Tue, Aug 28, 2012 at 04:42:20PM -0000, Gary Lasker wrote:
> Hi Michael, thanks for your review and your suggestions! I added the check for spinner_size and raise a ValueError as you suggested, and I've added a test case to test_spinner.py that checks 1. the small spinner case, 2. the large spinner case (as default) and finally 3. a ValueError if a disallowed value for spinner_size is passed in.

Great, thanks for the tests for this. I merged it into trunk now.

> I initially considered adding the actual sizes to enums.py as you suggested, or even letting them be specified directly in pixels, but I decided that I really don't forsee us needing any more than these two sizes - the large one for full views and the smaller one for subpanels. That, plus given that we really don't want to make it easy to create arbitrarily-sized spinner panels, I thought it best to keep it to the two useful choices and just embed the pixel details inside the class.

Ok, if we ever need to change it that will be straightfoward and until
then it stays as it is.

Cheers,
 Michael

=== modified file 'softwarecenter/ui/gtk3/widgets/containers.py'
--- softwarecenter/ui/gtk3/widgets/containers.py	2012-05-30 18:39:55 +0000
+++ softwarecenter/ui/gtk3/widgets/containers.py	2012-08-28 16:36:20 +0000
@@ -9,7 +9,8 @@
 from buttons import MoreLink
 from softwarecenter.ui.gtk3.em import StockEms
 from softwarecenter.ui.gtk3.drawing import rounded_rect
-from softwarecenter.ui.gtk3.widgets.spinner import SpinnerNotebook
+from softwarecenter.ui.gtk3.widgets.spinner import (SpinnerView,
+                                                    SpinnerNotebook)
 
 
 class FlowableGrid(Gtk.Fixed):
@@ -450,7 +451,8 @@
         self.content_box = Gtk.Box.new(orientation, spacing)
         self.content_box.show()
         # finally, a notebook for the spinner and the content box to share
-        self.spinner_notebook = SpinnerNotebook(self.content_box)
+        self.spinner_notebook = SpinnerNotebook(self.content_box,
+                                                spinner_size=SpinnerView.SMALL)
         self.box.add(self.spinner_notebook)
 
     def on_draw(self, cr):

=== modified file 'softwarecenter/ui/gtk3/widgets/spinner.py'
--- softwarecenter/ui/gtk3/widgets/spinner.py	2012-05-30 18:39:55 +0000
+++ softwarecenter/ui/gtk3/widgets/spinner.py	2012-08-28 16:36:20 +0000
@@ -27,15 +27,25 @@
 class SpinnerView(Gtk.Viewport):
     """A panel that contains a spinner with an optional legend.
 
-    The spinner is preset to a standard size and centered. An optional
-    label_text value can be specified for display with the spinner.
+    The spinner can be specified in one of two sizes, and defaults to
+    the larger size. An optional label_text value can be specified for
+    display with the spinner.
 
     """
+    # define spinner size options
+    (LARGE,
+     SMALL) = range(2)
 
-    def __init__(self, label_text=""):
+    def __init__(self, label_text="", spinner_size=LARGE):
         Gtk.Viewport.__init__(self)
         self.spinner = Gtk.Spinner()
-        self.spinner.set_size_request(48, 48)
+        if spinner_size not in (self.SMALL, self.LARGE):
+            raise ValueError('The value of spinner_size must be '
+                             'one of SpinnerView.SMALL or SpinnerView.LARGE')
+        if spinner_size == self.LARGE:
+            self.spinner.set_size_request(48, 48)
+        else:
+            self.spinner.set_size_request(24, 24)
 
         # use a table for the spinner (otherwise the spinner is massive!)
         spinner_table = Gtk.Table(3, 3, False)
@@ -77,10 +87,10 @@
     (CONTENT_PAGE,
      SPINNER_PAGE) = range(2)
 
-    def __init__(self, content, msg=""):
+    def __init__(self, content, msg="", spinner_size=SpinnerView.LARGE):
         Gtk.Notebook.__init__(self)
         self._last_timeout_id = None
-        self.spinner_view = SpinnerView(msg)
+        self.spinner_view = SpinnerView(msg, spinner_size)
         # its critical to show() the spinner early as otherwise
         # gtk_notebook_set_active_page() will not switch to it
         self.spinner_view.show()

=== modified file 'tests/gtk3/test_spinner.py'
--- tests/gtk3/test_spinner.py	2012-05-30 18:39:55 +0000
+++ tests/gtk3/test_spinner.py	2012-08-28 16:36:20 +0000
@@ -147,6 +147,24 @@
         self.assertFalse(self.obj.spinner_view.spinner.get_property('active'))
         self.assertFalse(self.obj.spinner_view.spinner.get_visible())
 
+    def test_specify_spinner_size(self):
+        # check small spinner case
+        notebook = spinner.SpinnerNotebook(self.content,
+                                           "test spinner label",
+                                           spinner.SpinnerView.SMALL)
+        self.assertEqual(notebook.spinner_view.spinner.get_size_request(),
+                        (24, 24))
+        # check large spinner case (default)
+        notebook = spinner.SpinnerNotebook(self.content,
+                                           "test spinner label")
+        self.assertEqual(notebook.spinner_view.spinner.get_size_request(),
+                        (48, 48))
+        # check that specifying an invalid spinner_size value raises
+        # an exception
+        with self.assertRaises(ValueError):
+            notebook = spinner.SpinnerNotebook(self.content,
+                                               "test spinner label",
+                                               48)
 
 if __name__ == "__main__":
     unittest.main()