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

Proposed by Gary Lasker
Status: Merged
Merged at revision: 3129
Proposed branch: lp:~gary-lasker/software-center/recommender-panel-visual-tweaks
Merge into: lp:software-center
Diff against target: 99 lines (+38/-8)
3 files modified
softwarecenter/ui/gtk3/widgets/containers.py (+4/-2)
softwarecenter/ui/gtk3/widgets/spinner.py (+16/-6)
tests/gtk3/test_spinner.py (+18/-0)
To merge this branch: bzr merge lp:~gary-lasker/software-center/recommender-panel-visual-tweaks
Reviewer Review Type Date Requested Status
software-store-developers Pending
Review via email: mp+121534@code.launchpad.net

Commit message

* 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

Description of the change

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!

To post a comment you must log in.
Michael Vogt (mvo) wrote :

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.

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) 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.

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'softwarecenter/ui/gtk3/widgets/containers.py'
2--- softwarecenter/ui/gtk3/widgets/containers.py 2012-05-30 18:39:55 +0000
3+++ softwarecenter/ui/gtk3/widgets/containers.py 2012-08-28 16:36:20 +0000
4@@ -9,7 +9,8 @@
5 from buttons import MoreLink
6 from softwarecenter.ui.gtk3.em import StockEms
7 from softwarecenter.ui.gtk3.drawing import rounded_rect
8-from softwarecenter.ui.gtk3.widgets.spinner import SpinnerNotebook
9+from softwarecenter.ui.gtk3.widgets.spinner import (SpinnerView,
10+ SpinnerNotebook)
11
12
13 class FlowableGrid(Gtk.Fixed):
14@@ -450,7 +451,8 @@
15 self.content_box = Gtk.Box.new(orientation, spacing)
16 self.content_box.show()
17 # finally, a notebook for the spinner and the content box to share
18- self.spinner_notebook = SpinnerNotebook(self.content_box)
19+ self.spinner_notebook = SpinnerNotebook(self.content_box,
20+ spinner_size=SpinnerView.SMALL)
21 self.box.add(self.spinner_notebook)
22
23 def on_draw(self, cr):
24
25=== modified file 'softwarecenter/ui/gtk3/widgets/spinner.py'
26--- softwarecenter/ui/gtk3/widgets/spinner.py 2012-05-30 18:39:55 +0000
27+++ softwarecenter/ui/gtk3/widgets/spinner.py 2012-08-28 16:36:20 +0000
28@@ -27,15 +27,25 @@
29 class SpinnerView(Gtk.Viewport):
30 """A panel that contains a spinner with an optional legend.
31
32- The spinner is preset to a standard size and centered. An optional
33- label_text value can be specified for display with the spinner.
34+ The spinner can be specified in one of two sizes, and defaults to
35+ the larger size. An optional label_text value can be specified for
36+ display with the spinner.
37
38 """
39+ # define spinner size options
40+ (LARGE,
41+ SMALL) = range(2)
42
43- def __init__(self, label_text=""):
44+ def __init__(self, label_text="", spinner_size=LARGE):
45 Gtk.Viewport.__init__(self)
46 self.spinner = Gtk.Spinner()
47- self.spinner.set_size_request(48, 48)
48+ if spinner_size not in (self.SMALL, self.LARGE):
49+ raise ValueError('The value of spinner_size must be '
50+ 'one of SpinnerView.SMALL or SpinnerView.LARGE')
51+ if spinner_size == self.LARGE:
52+ self.spinner.set_size_request(48, 48)
53+ else:
54+ self.spinner.set_size_request(24, 24)
55
56 # use a table for the spinner (otherwise the spinner is massive!)
57 spinner_table = Gtk.Table(3, 3, False)
58@@ -77,10 +87,10 @@
59 (CONTENT_PAGE,
60 SPINNER_PAGE) = range(2)
61
62- def __init__(self, content, msg=""):
63+ def __init__(self, content, msg="", spinner_size=SpinnerView.LARGE):
64 Gtk.Notebook.__init__(self)
65 self._last_timeout_id = None
66- self.spinner_view = SpinnerView(msg)
67+ self.spinner_view = SpinnerView(msg, spinner_size)
68 # its critical to show() the spinner early as otherwise
69 # gtk_notebook_set_active_page() will not switch to it
70 self.spinner_view.show()
71
72=== modified file 'tests/gtk3/test_spinner.py'
73--- tests/gtk3/test_spinner.py 2012-05-30 18:39:55 +0000
74+++ tests/gtk3/test_spinner.py 2012-08-28 16:36:20 +0000
75@@ -147,6 +147,24 @@
76 self.assertFalse(self.obj.spinner_view.spinner.get_property('active'))
77 self.assertFalse(self.obj.spinner_view.spinner.get_visible())
78
79+ def test_specify_spinner_size(self):
80+ # check small spinner case
81+ notebook = spinner.SpinnerNotebook(self.content,
82+ "test spinner label",
83+ spinner.SpinnerView.SMALL)
84+ self.assertEqual(notebook.spinner_view.spinner.get_size_request(),
85+ (24, 24))
86+ # check large spinner case (default)
87+ notebook = spinner.SpinnerNotebook(self.content,
88+ "test spinner label")
89+ self.assertEqual(notebook.spinner_view.spinner.get_size_request(),
90+ (48, 48))
91+ # check that specifying an invalid spinner_size value raises
92+ # an exception
93+ with self.assertRaises(ValueError):
94+ notebook = spinner.SpinnerNotebook(self.content,
95+ "test spinner label",
96+ 48)
97
98 if __name__ == "__main__":
99 unittest.main()

Subscribers

People subscribed via source and target branches