Code review comment for lp:~ralsina/ubuntuone-control-panel/tab-tab-tab

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