Merge lp:~ralsina/ubuntuone-control-panel/tab-tab-tab into lp:ubuntuone-control-panel

Proposed by Roberto Alsina on 2012-03-09
Status: Merged
Approved by: Roberto Alsina on 2012-03-12
Approved revision: 291
Merged at revision: 283
Proposed branch: lp:~ralsina/ubuntuone-control-panel/tab-tab-tab
Merge into: lp:ubuntuone-control-panel
Diff against target: 239 lines (+107/-16)
2 files modified
ubuntuone/controlpanel/gui/qt/folders.py (+53/-9)
ubuntuone/controlpanel/gui/qt/tests/test_folders.py (+54/-7)
To merge this branch: bzr merge lp:~ralsina/ubuntuone-control-panel/tab-tab-tab
Reviewer Review Type Date Requested Status
Diego Sarmentero (community) Approve on 2012-03-12
Natalia Bidart 2012-03-09 Approve on 2012-03-12
Review via email: mp+96687@code.launchpad.net

Commit message

- Arranged Tab ordering in folders tab according to guidelines (LP: #950073).

Description of the change

- Arranged Tab ordering in folders tab according to guidelines (LP: #950073).

To post a comment you must log in.
287. By Roberto Alsina on 2012-03-09

merged trunk

Natalia Bidart (nataliabidart) wrote :

* I think this code is a leftover from the merge with trunk, but it should be removed:

                # Operator not preceded by a space
                # pylint: disable=C0322
                cb = lambda checked, item=child: \
                    self.on_folders_itemActivated(item)
                # pylint: enable=C0322
                button.clicked.connect(cb)

* Question, why are you adding self.is_processing = True? that code triggers the showing of the loading overlay which is already being shown in load() and hidden in the line 145 of process_info.

* In test_focus_order, could you please not use literal but the value from the FAKE_VOLUMES_INFO stub data? Ideally you should iterate the FAKE_VOLUMES_INFO so if we change it to add a specific buggy entry, this test does not break.

* Any reason to use this style?

                self.assertEqual(self.ui.widget_items[button],
                item)

instead of:

                self.assertEqual(self.ui.widget_items[button], item)

since line length allows it to be like the second form.

After testing IRL, it works great! (tough because the leftover callback, the file manager is opened twice).

review: Needs Fixing
288. By Roberto Alsina on 2012-03-09

removed remnant

289. By Roberto Alsina on 2012-03-09

removed remnant, extra guard, style fix

Roberto Alsina (ralsina) wrote :

> * I think this code is a leftover from the merge with trunk, but it should be
> removed:
>
> # Operator not preceded by a space
> # pylint: disable=C0322
> cb = lambda checked, item=child: \
> self.on_folders_itemActivated(item)
> # pylint: enable=C0322
> button.clicked.connect(cb)

Yes, bad merge with no conflict warning, will remove.

> * Question, why are you adding self.is_processing = True? that code triggers
> the showing of the loading overlay which is already being shown in load() and
> hidden in the line 145 of process_info.

I was getting weird stuff when not setting that.Turns out I was missing a guard in on_folders_itemChanged instead.

> * In test_focus_order, could you please not use literal but the value from the
> FAKE_VOLUMES_INFO stub data? Ideally you should iterate the FAKE_VOLUMES_INFO
> so if we change it to add a specific buggy entry, this test does not break.

Tricky. The items are not in the same order in FAKE_VOLUMES_INFO and in the widget, so any change in FAKE_VOLUMES_INFO can change what we get in test_focus_order. I could do a loop, over a VOLUMES_INFO
that was sorted, and not use FAKE_VOLUMES_INFO.

>
> * Any reason to use this style?
>
> self.assertEqual(self.ui.widget_items[button],
> item)
>
> instead of:
>
> self.assertEqual(self.ui.widget_items[button], item)
>
> since line length allows it to be like the second form.

It used to be longer ;-)

I pushed the suggested changes (except for the FAKE_VOLUMES_INFO loop one) in revno 289.

290. By Roberto Alsina on 2012-03-12

removed commented code

291. By Roberto Alsina on 2012-03-12

removed useless line

Natalia Bidart (nataliabidart) wrote :

Looks good!

review: Approve
Diego Sarmentero (diegosarmentero) wrote :

+1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntuone/controlpanel/gui/qt/folders.py'
2--- ubuntuone/controlpanel/gui/qt/folders.py 2012-03-09 17:11:54 +0000
3+++ ubuntuone/controlpanel/gui/qt/folders.py 2012-03-12 14:58:17 +0000
4@@ -81,6 +81,7 @@
5
6 ui_class = folders_ui
7 logger = logger
8+ widget_items = {}
9
10 def _setup(self):
11 """Do some extra setupping for the UI."""
12@@ -101,6 +102,14 @@
13 self.ui.share_publish_button.uri = MANAGE_FILES_LINK
14 icon = icon_from_name('external_icon_orange')
15 self.ui.share_publish_button.setIcon(icon)
16+ QtGui.QApplication.instance().focusChanged.connect(self.focus_changed)
17+
18+ def focus_changed(self, old, new):
19+ """When a inner widget is focused, scroll to its item."""
20+ item = self.widget_items.get(new, None)
21+ if item is not None:
22+ self.ui.folders.scrollToItem(item)
23+ self.ui.folders.setCurrentItem(item)
24
25 @log_call(logger.info)
26 def on_folder_created(self, new_folder):
27@@ -132,6 +141,7 @@
28 # the code would be much more complicated.
29 scrollbar_position = self.ui.folders.verticalScrollBar().value()
30 self.ui.folders.clear()
31+ self.widget_items = {}
32 self.is_processing = False
33
34 for name, _, volumes in info: # ignore free_bytes
35@@ -177,32 +187,63 @@
36 icon_name = MUSIC_ICON_NAME
37
38 icon = icon_from_name(icon_name)
39+ child.icon_obj = icon # hack!
40+ child.setIcon(FOLDER_NAME_COL, icon)
41+ item.addChild(child)
42
43 if is_root:
44 child.setText(SUBSCRIPTION_COL, ALWAYS_SUBSCRIBED)
45 else: # set check state
46+ # We are using an embedded checkbox instead of the
47+ # item's checkState, because of focus and navigation
48+ # issues.
49+ checkbox = QtGui.QCheckBox(parent=self.ui.folders)
50+ self.widget_items[checkbox] = child
51 if bool(volume[u'subscribed']):
52- child.setCheckState(SUBSCRIPTION_COL, CHECKED)
53+ checkbox.setCheckState(CHECKED)
54 else:
55- child.setCheckState(SUBSCRIPTION_COL, UNCHECKED)
56+ checkbox.setCheckState(UNCHECKED)
57 pixmap = icon.pixmap(24, icon.Disabled, icon.Off)
58 icon = QtGui.QIcon(pixmap)
59 icon.icon_name = icon_name
60-
61- child.icon_obj = icon # hack!
62- child.setIcon(FOLDER_NAME_COL, icon)
63- item.addChild(child)
64+ self.ui.folders.setItemWidget(child, SUBSCRIPTION_COL,
65+ checkbox)
66+
67+ # Operator not preceded by a space
68+ # pylint: disable=C0322
69+ cb = lambda checked, item=child: \
70+ self.on_folders_itemChanged(item, SUBSCRIPTION_COL)
71+ # pylint: enable=C0322
72+
73+ checkbox.stateChanged.connect(cb)
74
75 # attach a third item with a button to explore the folder
76- model_index = self.ui.folders.indexFromItem(child, EXPLORE_COL)
77 button = ExploreFolderButton(folder_path=child.volume_path,
78 parent=self.ui.folders)
79+ self.widget_items[button] = child
80 button.setEnabled(bool(volume[u'subscribed']))
81- self.ui.folders.setIndexWidget(model_index, button)
82+ self.ui.folders.setItemWidget(child, EXPLORE_COL, button)
83
84 self.ui.folders.expandAll()
85 self.ui.folders.verticalScrollBar().setValue(scrollbar_position)
86
87+ # Rearrange the focus chain so that explore buttons
88+ # and checkboxes are right after ui.folders, and in
89+ # the displayed order.
90+ previous_widget = self.ui.folders
91+ it = QtGui.QTreeWidgetItemIterator(self.ui.folders)
92+ while it.value():
93+ item = it.value()
94+ checkbox = self.ui.folders.itemWidget(item, SUBSCRIPTION_COL)
95+ button = self.ui.folders.itemWidget(item, EXPLORE_COL)
96+ if checkbox:
97+ QtGui.QWidget.setTabOrder(previous_widget, checkbox)
98+ previous_widget = checkbox
99+ if button:
100+ QtGui.QWidget.setTabOrder(previous_widget, button)
101+ previous_widget = button
102+ it += 1
103+
104 # Invalid name "on_folders_itemActivated", "on_folders_itemChanged"
105 # pylint: disable=C0103
106
107@@ -233,7 +274,10 @@
108 # ignore signals when the UI is being updated
109 return
110
111- subscribed = item.checkState(SUBSCRIPTION_COL) == CHECKED # new state
112+ checkbox = self.ui.folders.itemWidget(item, SUBSCRIPTION_COL)
113+ if not checkbox:
114+ return
115+ subscribed = checkbox.checkState() == CHECKED # new state
116 logger.info('on_folders_itemChanged: processing volume id %r with '
117 'path %r, new subscribed value is: %r. Path exists? %r',
118 volume_id, volume_path, subscribed,
119
120=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_folders.py'
121--- ubuntuone/controlpanel/gui/qt/tests/test_folders.py 2012-03-08 20:46:13 +0000
122+++ ubuntuone/controlpanel/gui/qt/tests/test_folders.py 2012-03-12 14:58:17 +0000
123@@ -20,6 +20,7 @@
124 import operator
125 import os
126
127+from PyQt4 import QtGui
128 from twisted.internet import defer
129 from ubuntuone.devtools.handlers import MementoHandler
130
131@@ -38,7 +39,6 @@
132 UbuntuOneBinTestCase,
133 )
134
135-
136 # Access to a protected member
137 # Instance of 'ControlBackend' has no '_called' member
138 # pylint: disable=W0212, E1103
139@@ -171,8 +171,9 @@
140 self.assertEqual(item.text(gui.SUBSCRIPTION_COL),
141 gui.ALWAYS_SUBSCRIBED)
142 else:
143- subscribed = item.checkState(gui.SUBSCRIPTION_COL) == \
144- gui.CHECKED
145+ checkbox = self.ui.ui.folders.itemWidget(
146+ item, gui.SUBSCRIPTION_COL)
147+ subscribed = checkbox.checkState() == gui.CHECKED
148 self.assertEqual(subscribed, bool(volume['subscribed']))
149
150 icon_name = item.icon_obj.icon_name
151@@ -321,7 +322,8 @@
152 label = item.text(gui.FOLDER_NAME_COL)
153 self.assertEqual(label, gui.MUSIC_DISPLAY_NAME)
154
155- subscribed = item.checkState(gui.SUBSCRIPTION_COL) == gui.CHECKED
156+ checkbox = self.ui.ui.folders.itemWidget(item, gui.SUBSCRIPTION_COL)
157+ subscribed = checkbox.checkState() == gui.CHECKED
158 self.assertEqual(subscribed, bool(volume['subscribed']))
159
160 icon_name = item.icon_obj.icon_name
161@@ -335,6 +337,45 @@
162 self.assert_uri_hook_called(self.ui.ui.share_publish_button,
163 gui.MANAGE_FILES_LINK)
164
165+ def test_focus_order(self):
166+ """Ensure that the inner widgets are in the correct tab order."""
167+ self.ui.process_info(FAKE_VOLUMES_INFO)
168+ # First, assert we are jumping from ui.folders to an
169+ # QPushButton
170+ widget = self.ui.ui.folders.nextInFocusChain()
171+ self.assertIsInstance(widget, QtGui.QPushButton)
172+ # Then, assert it's the right one
173+ self.assertEqual(unicode(
174+ self.ui.widget_items[widget].text(0)), u"My Ubuntu")
175+ # Next are a checkbox / pushbutton pair
176+ # in the 'bar' item
177+ widget = widget.nextInFocusChain()
178+ self.assertIsInstance(widget, QtGui.QCheckBox)
179+ self.assertEqual(unicode(
180+ self.ui.widget_items[widget].text(0)), u"bar")
181+ widget = widget.nextInFocusChain()
182+ self.assertIsInstance(widget, QtGui.QPushButton)
183+ self.assertEqual(unicode(
184+ self.ui.widget_items[widget].text(0)), u"bar")
185+
186+ def test_widget_dict(self):
187+ """Ensure the widget_items dictionary is full."""
188+ self.ui.process_info(FAKE_VOLUMES_INFO)
189+ it = QtGui.QTreeWidgetItemIterator(self.ui.ui.folders)
190+ while it.value():
191+ item = it.value()
192+ checkbox = self.ui.ui.folders.itemWidget(item,
193+ gui.SUBSCRIPTION_COL)
194+ button = self.ui.ui.folders.itemWidget(item,
195+ gui.EXPLORE_COL)
196+ if checkbox:
197+ self.assertEqual(self.ui.widget_items[checkbox],
198+ item)
199+ if button:
200+ self.assertEqual(self.ui.widget_items[button],
201+ item)
202+ it += 1
203+
204
205 class FoldersPanelAddFolderTestCase(FoldersPanelTestCase):
206 """The test suite for the folder creation from a local dir."""
207@@ -389,7 +430,9 @@
208 check_state = gui.CHECKED if subscribed else gui.UNCHECKED
209
210 self.ui.is_processing = True
211- self.item.setCheckState(gui.SUBSCRIPTION_COL, check_state)
212+ checkbox = self.ui.ui.folders.itemWidget(self.item,
213+ gui.SUBSCRIPTION_COL)
214+ checkbox.setCheckState(check_state)
215 self.ui.is_processing = False
216
217 yield self.ui.on_folders_itemChanged(self.item)
218@@ -398,7 +441,9 @@
219 self.assert_backend_called('change_volume_settings',
220 fid, {'subscribed': subscribed})
221
222- value = self.item.checkState(gui.SUBSCRIPTION_COL) == gui.CHECKED
223+ checkbox = self.ui.ui.folders.itemWidget(self.item,
224+ gui.SUBSCRIPTION_COL)
225+ value = checkbox.checkState() == gui.CHECKED
226 self.assertEqual(value, bool(subscribed))
227
228 # folder list was reloaded
229@@ -518,7 +563,9 @@
230 """The confirmation dialog is not shown if unsubscribing."""
231 # make sure the item is unsubscribed
232 self.ui.is_processing = True
233- self.item.setCheckState(gui.SUBSCRIPTION_COL, gui.UNCHECKED)
234+ checkbox = self.ui.ui.folders.itemWidget(
235+ self.item, gui.SUBSCRIPTION_COL)
236+ checkbox.setCheckState(gui.UNCHECKED)
237 self.ui.is_processing = False
238
239 # the confirm dialog was not called so far

Subscribers

People subscribed via source and target branches