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

[Roberto Alsina]

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

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

[lp:~ralsina/ubuntuone-control-panel/tab-tab-tab]

287. By Roberto Alsina on 2012-03-09

merged trunk

[Natalia Bidart (nataliabidart)]

* 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

[lp:~ralsina/ubuntuone-control-panel/tab-tab-tab]

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

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

[lp:~ralsina/ubuntuone-control-panel/tab-tab-tab]

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

Looks good!

review: Approve

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

=== modified file 'ubuntuone/controlpanel/gui/qt/folders.py'
--- ubuntuone/controlpanel/gui/qt/folders.py	2012-03-09 17:11:54 +0000
+++ ubuntuone/controlpanel/gui/qt/folders.py	2012-03-12 14:58:17 +0000
@@ -81,6 +81,7 @@
 
     ui_class = folders_ui
     logger = logger
+    widget_items = {}
 
     def _setup(self):
         """Do some extra setupping for the UI."""
@@ -101,6 +102,14 @@
         self.ui.share_publish_button.uri = MANAGE_FILES_LINK
         icon = icon_from_name('external_icon_orange')
         self.ui.share_publish_button.setIcon(icon)
+        QtGui.QApplication.instance().focusChanged.connect(self.focus_changed)
+
+    def focus_changed(self, old, new):
+        """When a inner widget is focused, scroll to its item."""
+        item = self.widget_items.get(new, None)
+        if item is not None:
+            self.ui.folders.scrollToItem(item)
+            self.ui.folders.setCurrentItem(item)
 
     @log_call(logger.info)
     def on_folder_created(self, new_folder):
@@ -132,6 +141,7 @@
         # the code would be much more complicated.
         scrollbar_position = self.ui.folders.verticalScrollBar().value()
         self.ui.folders.clear()
+        self.widget_items = {}
         self.is_processing = False
 
         for name, _, volumes in info:  # ignore free_bytes
@@ -177,32 +187,63 @@
                     icon_name = MUSIC_ICON_NAME
 
                 icon = icon_from_name(icon_name)
+                child.icon_obj = icon  # hack!
+                child.setIcon(FOLDER_NAME_COL, icon)
+                item.addChild(child)
 
                 if is_root:
                     child.setText(SUBSCRIPTION_COL, ALWAYS_SUBSCRIBED)
                 else:  # set check state
+                    # We are using an embedded checkbox instead of the
+                    # item's checkState, because of focus and navigation
+                    # issues.
+                    checkbox = QtGui.QCheckBox(parent=self.ui.folders)
+                    self.widget_items[checkbox] = child
                     if bool(volume[u'subscribed']):
-                        child.setCheckState(SUBSCRIPTION_COL, CHECKED)
+                        checkbox.setCheckState(CHECKED)
                     else:
-                        child.setCheckState(SUBSCRIPTION_COL, UNCHECKED)
+                        checkbox.setCheckState(UNCHECKED)
                         pixmap = icon.pixmap(24, icon.Disabled, icon.Off)
                         icon = QtGui.QIcon(pixmap)
                         icon.icon_name = icon_name
-
-                child.icon_obj = icon  # hack!
-                child.setIcon(FOLDER_NAME_COL, icon)
-                item.addChild(child)
+                    self.ui.folders.setItemWidget(child, SUBSCRIPTION_COL,
+                        checkbox)
+
+                    # Operator not preceded by a space
+                    # pylint: disable=C0322
+                    cb = lambda checked, item=child: \
+                        self.on_folders_itemChanged(item, SUBSCRIPTION_COL)
+                    # pylint: enable=C0322
+
+                    checkbox.stateChanged.connect(cb)
 
                 # attach a third item with a button to explore the folder
-                model_index = self.ui.folders.indexFromItem(child, EXPLORE_COL)
                 button = ExploreFolderButton(folder_path=child.volume_path,
                             parent=self.ui.folders)
+                self.widget_items[button] = child
                 button.setEnabled(bool(volume[u'subscribed']))
-                self.ui.folders.setIndexWidget(model_index, button)
+                self.ui.folders.setItemWidget(child, EXPLORE_COL, button)
 
         self.ui.folders.expandAll()
         self.ui.folders.verticalScrollBar().setValue(scrollbar_position)
 
+        # Rearrange the focus chain so that explore buttons
+        # and checkboxes are right after ui.folders, and in
+        # the displayed order.
+        previous_widget = self.ui.folders
+        it = QtGui.QTreeWidgetItemIterator(self.ui.folders)
+        while it.value():
+            item = it.value()
+            checkbox = self.ui.folders.itemWidget(item, SUBSCRIPTION_COL)
+            button = self.ui.folders.itemWidget(item, EXPLORE_COL)
+            if checkbox:
+                QtGui.QWidget.setTabOrder(previous_widget, checkbox)
+                previous_widget = checkbox
+            if button:
+                QtGui.QWidget.setTabOrder(previous_widget, button)
+                previous_widget = button
+            it += 1
+
     # Invalid name "on_folders_itemActivated", "on_folders_itemChanged"
     # pylint: disable=C0103
 
@@ -233,7 +274,10 @@
             # ignore signals when the UI is being updated
             return
 
-        subscribed = item.checkState(SUBSCRIPTION_COL) == CHECKED  # new state
+        checkbox = self.ui.folders.itemWidget(item, SUBSCRIPTION_COL)
+        if not checkbox:
+            return
+        subscribed = checkbox.checkState() == CHECKED  # new state
         logger.info('on_folders_itemChanged: processing volume id %r with '
                     'path %r, new subscribed value is: %r. Path exists? %r',
                     volume_id, volume_path, subscribed,

=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_folders.py'
--- ubuntuone/controlpanel/gui/qt/tests/test_folders.py	2012-03-08 20:46:13 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/test_folders.py	2012-03-12 14:58:17 +0000
@@ -20,6 +20,7 @@
 import operator
 import os
 
+from PyQt4 import QtGui
 from twisted.internet import defer
 from ubuntuone.devtools.handlers import MementoHandler
 
@@ -38,7 +39,6 @@
     UbuntuOneBinTestCase,
 )
 
-
 # Access to a protected member
 # Instance of 'ControlBackend' has no '_called' member
 # pylint: disable=W0212, E1103
@@ -171,8 +171,9 @@
                     self.assertEqual(item.text(gui.SUBSCRIPTION_COL),
                                      gui.ALWAYS_SUBSCRIBED)
                 else:
-                    subscribed = item.checkState(gui.SUBSCRIPTION_COL) == \
-                                 gui.CHECKED
+                    checkbox = self.ui.ui.folders.itemWidget(
+                        item, gui.SUBSCRIPTION_COL)
+                    subscribed = checkbox.checkState() == gui.CHECKED
                     self.assertEqual(subscribed, bool(volume['subscribed']))
 
                 icon_name = item.icon_obj.icon_name
@@ -321,7 +322,8 @@
         label = item.text(gui.FOLDER_NAME_COL)
         self.assertEqual(label, gui.MUSIC_DISPLAY_NAME)
 
-        subscribed = item.checkState(gui.SUBSCRIPTION_COL) == gui.CHECKED
+        checkbox = self.ui.ui.folders.itemWidget(item, gui.SUBSCRIPTION_COL)
+        subscribed = checkbox.checkState() == gui.CHECKED
         self.assertEqual(subscribed, bool(volume['subscribed']))
 
         icon_name = item.icon_obj.icon_name
@@ -335,6 +337,45 @@
         self.assert_uri_hook_called(self.ui.ui.share_publish_button,
                                     gui.MANAGE_FILES_LINK)
 
+    def test_focus_order(self):
+        """Ensure that the inner widgets are in the correct tab order."""
+        self.ui.process_info(FAKE_VOLUMES_INFO)
+        # First, assert we are jumping from ui.folders to an
+        # QPushButton
+        widget = self.ui.ui.folders.nextInFocusChain()
+        self.assertIsInstance(widget, QtGui.QPushButton)
+        # Then, assert it's the right one
+        self.assertEqual(unicode(
+            self.ui.widget_items[widget].text(0)), u"My Ubuntu")
+        # Next are a checkbox / pushbutton pair
+        # in the 'bar' item
+        widget = widget.nextInFocusChain()
+        self.assertIsInstance(widget, QtGui.QCheckBox)
+        self.assertEqual(unicode(
+            self.ui.widget_items[widget].text(0)), u"bar")
+        widget = widget.nextInFocusChain()
+        self.assertIsInstance(widget, QtGui.QPushButton)
+        self.assertEqual(unicode(
+            self.ui.widget_items[widget].text(0)), u"bar")
+
+    def test_widget_dict(self):
+        """Ensure the widget_items dictionary is full."""
+        self.ui.process_info(FAKE_VOLUMES_INFO)
+        it = QtGui.QTreeWidgetItemIterator(self.ui.ui.folders)
+        while it.value():
+            item = it.value()
+            checkbox = self.ui.ui.folders.itemWidget(item,
+                gui.SUBSCRIPTION_COL)
+            button = self.ui.ui.folders.itemWidget(item,
+                gui.EXPLORE_COL)
+            if checkbox:
+                self.assertEqual(self.ui.widget_items[checkbox],
+                item)
+            if button:
+                self.assertEqual(self.ui.widget_items[button],
+                item)
+            it += 1
+
 
 class FoldersPanelAddFolderTestCase(FoldersPanelTestCase):
     """The test suite for the folder creation from a local dir."""
@@ -389,7 +430,9 @@
         check_state = gui.CHECKED if subscribed else gui.UNCHECKED
 
         self.ui.is_processing = True
-        self.item.setCheckState(gui.SUBSCRIPTION_COL, check_state)
+        checkbox = self.ui.ui.folders.itemWidget(self.item,
+            gui.SUBSCRIPTION_COL)
+        checkbox.setCheckState(check_state)
         self.ui.is_processing = False
 
         yield self.ui.on_folders_itemChanged(self.item)
@@ -398,7 +441,9 @@
         self.assert_backend_called('change_volume_settings',
                                    fid, {'subscribed': subscribed})
 
-        value = self.item.checkState(gui.SUBSCRIPTION_COL) == gui.CHECKED
+        checkbox = self.ui.ui.folders.itemWidget(self.item,
+            gui.SUBSCRIPTION_COL)
+        value = checkbox.checkState() == gui.CHECKED
         self.assertEqual(value, bool(subscribed))
 
         # folder list was reloaded
@@ -518,7 +563,9 @@
         """The confirmation dialog is not shown if unsubscribing."""
         # make sure the item is unsubscribed
         self.ui.is_processing = True
-        self.item.setCheckState(gui.SUBSCRIPTION_COL, gui.UNCHECKED)
+        checkbox = self.ui.ui.folders.itemWidget(
+            self.item, gui.SUBSCRIPTION_COL)
+        checkbox.setCheckState(gui.UNCHECKED)
         self.ui.is_processing = False
 
         # the confirm dialog was not called so far