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

Revision history for this message
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

« Back to merge proposal