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

Proposed by Roberto Alsina
Status: Merged
Approved by: Roberto Alsina
Approved revision: 291
Merged at revision: 283
Proposed branch: lp://qastaging/~ralsina/ubuntuone-control-panel/tab-tab-tab
Merge into: lp://qastaging/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://qastaging/~ralsina/ubuntuone-control-panel/tab-tab-tab
Reviewer Review Type Date Requested Status
Diego Sarmentero (community) Approve
Natalia Bidart (community) Approve
Review via email: mp+96687@code.qastaging.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

merged trunk

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
288. By Roberto Alsina

removed remnant

289. By Roberto Alsina

removed remnant, extra guard, style fix

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

removed commented code

291. By Roberto Alsina

removed useless line

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

Looks good!

review: Approve
Revision history for this message
Diego Sarmentero (diegosarmentero) wrote :

+1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches