Merge lp://qastaging/~diegosarmentero/ubuntuone-control-panel/share-indicator into lp://qastaging/ubuntuone-control-panel

Proposed by Diego Sarmentero
Status: Merged
Approved by: dobey
Approved revision: 374
Merged at revision: 375
Proposed branch: lp://qastaging/~diegosarmentero/ubuntuone-control-panel/share-indicator
Merge into: lp://qastaging/ubuntuone-control-panel
Diff against target: 297 lines (+98/-125)
2 files modified
ubuntuone/controlpanel/gui/qt/systray.py (+53/-68)
ubuntuone/controlpanel/gui/qt/tests/test_systray.py (+45/-57)
To merge this branch: bzr merge lp://qastaging/~diegosarmentero/ubuntuone-control-panel/share-indicator
Reviewer Review Type Date Requested Status
dobey (community) Abstain
Mike McCracken (community) Approve
Michał Karnicki (community) Approve
Review via email: mp+131973@code.qastaging.launchpad.net

Commit message

- Accessing the Share tab from the systray menu and avoid refreshing the menu unnecesary (LP: #1072859 #1063781).

Description of the change

There is an option to access the Share Tab in the systray menu (you can test this on linux too executing the control panel with --with-icon), and the code for refreshing the systray menu was removed because the server doesn't have a proper api to access the info of new (not accepted yet) shares, so that part of the code and the need to refresh the menu was unnecesary.

To post a comment you must log in.
Revision history for this message
Mike McCracken (mikemc) wrote :

I think this should be two branches - adding the shares tab support is simple and obviously a good idea, but the part that removes the systray menu functionality for showing incoming shares needs more discussion, IMO.

I see that the current code isn't perfect - it only asks the local syncdaemon for the shares list, so we won't see new shares when we refresh. (And it doesn't appear that we're
However, instead of removing the functionality, can we fix it?

controlpanel.backend only has a get_shares call, but syncdaemontool has a refresh_shares call that gets the current list of shares from the server. Could we just add that and fix the feature? (I haven't tried it, so that's a real question)

review: Needs Information
Revision history for this message
Mike McCracken (mikemc) wrote :

After following up, I was told (by verterok) that there's no current way to map the email used for a share request to a current user id, so the shares list we get from the server won't ever have un-accepted shares in it.

So that answers my questions… I'm going to abstain just in case two people end up reviewing this before tomorrow. Otherwise I'll review it first thing tomorrow.

review: Abstain
Revision history for this message
Michał Karnicki (karni) wrote :

Looks good to me.

review: Approve
Revision history for this message
Mike McCracken (mikemc) wrote :

IRL on windows, the 'open ubuntu one' and 'share a file' items both don't do anything when the CP main window has been minimized...

Is there a way to get them to bring the window back out of minimization?

I think there was a similar discussion about the u1-client version of these changes…

review: Needs Fixing
Revision history for this message
dobey (dobey) wrote :
review: Needs Information
Revision history for this message
Diego Sarmentero (diegosarmentero) wrote :

> IRL on windows, the 'open ubuntu one' and 'share a file' items both don't do
> anything when the CP main window has been minimized...
>
> Is there a way to get them to bring the window back out of minimization?
>
> I think there was a similar discussion about the u1-client version of these
> changes…

Fixed in this other branch:

https://code.launchpad.net/~diegosarmentero/ubuntuone-control-panel/socket-communication/+merge/132409

Revision history for this message
Mike McCracken (mikemc) wrote :

> > IRL on windows, the 'open ubuntu one' and 'share a file' items both don't do
> > anything when the CP main window has been minimized...
> >
> > Is there a way to get them to bring the window back out of minimization?
> >
> > I think there was a similar discussion about the u1-client version of these
> > changes…
>
> Fixed in this other branch:
>
> https://code.launchpad.net/~diegosarmentero/ubuntuone-control-panel/socket-
> communication/+merge/132409

OK, that should fix my only concern.

review: Approve
Revision history for this message
dobey (dobey) :
review: Abstain

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