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

[Diego Sarmentero]

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

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.

[Mike McCracken (mikemc)]

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

[Mike McCracken (mikemc)]

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

[Michał Karnicki (karni)]

Looks good to me.

review: Approve

[Mike McCracken (mikemc)]

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

[dobey (dobey)]

Similar concerns as with https://code.launchpad.net/~diegosarmentero/ubuntuone-client/menu-new-options/+merge/132106

review: Needs Information

[Diego Sarmentero (diegosarmentero)]

> 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

[Mike McCracken (mikemc)]

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

[dobey (dobey)]

review: Abstain

=== modified file 'ubuntuone/controlpanel/gui/qt/systray.py'
--- ubuntuone/controlpanel/gui/qt/systray.py	2012-09-05 19:43:16 +0000
+++ ubuntuone/controlpanel/gui/qt/systray.py	2012-10-29 19:38:20 +0000
@@ -36,12 +36,12 @@
     IN_PROGRESS,
     IN_PROGRESS_FILE,
     LOADING,
-    NEW_SHARE_BY,
     OPEN_UBUNTU_ONE,
     OPEN_UBUNTU_ONE_FOLDER,
     PLEASE_WAIT,
     RECENT_TRANSFERS,
     RECENTTRANSFERS,
+    SHARE_A_FILE,
     TRANSFERS,
     UPLOADING,
 )
@@ -122,6 +122,7 @@
         self.quit = None
         self.get_help_online = None
         self.open_u1 = None
+        self.share_a_file = None
         self.status_action = None
         self.go_to_web = None
         self.get_more_storage = None
@@ -130,75 +131,48 @@
         self.backend.add_status_changed_handler(self._process_status)
 
         self.load_menu()
-        # Refresh the Shares every five minutes if needed.
-        self._timer_id = self.startTimer(300000)
-
-    # pylint: disable=C0103
-
-    def timerEvent(self, event):
-        """Update the menu on each iteration."""
-        self.load_menu()
-
-    # pylint: enable=C0103
-
-    @inlineCallbacks
+
     def load_menu(self):
         """Load the content of the menu."""
-        shares_info = yield self.backend.get_shares()
-        shares_info = [share for share in shares_info if not share['accepted']]
-        if shares_info != self._previous_shares:
-            # Items
-            self.context_menu.clear()
-
-            self.status = self.context_menu.addAction(LOADING)
-            self.status.setEnabled(False)
-            self.status_action = self.context_menu.addAction(PLEASE_WAIT)
-            self.refresh_status()
-            self.context_menu.addSeparator()
-
-            self.open_u1 = self.context_menu.addAction(OPEN_UBUNTU_ONE)
-            # TODO: Share a file action when the Shares tab is ready in U1-CP
-            self.open_u1_folder = self.context_menu.addAction(
-                OPEN_UBUNTU_ONE_FOLDER)
-            self.go_to_web = self.context_menu.addAction(GO_TO_WEB)
-            self.context_menu.addSeparator()
-
-            # Shares
-            self._previous_shares = shares_info
-            max_shares = 0
-            for share in self._previous_shares:
-                if max_shares == 3:
-                    break
-                max_shares += 1
-                text = NEW_SHARE_BY % share['other_visible_name']
-                share_action = SharesAction(text, share['volume_id'],
-                    self.context_menu)
-                self.context_menu.addAction(share_action)
-            if self._previous_shares:
-                self.context_menu.addSeparator()
-
-            # Transfers
-            self.transfers = TransfersMenu(self)
-            self.context_menu.addMenu(self.transfers)
-
-            self.get_more_storage = self.context_menu.addAction(
-                GET_MORE_STORAGE)
-            self.get_help_online = self.context_menu.addAction(GET_HELP_ONLINE)
-            self.quit = self.context_menu.addAction("Quit")
-
-            self.setContextMenu(self.context_menu)
-
-            # Connect Signals
-            self.status_action.triggered.connect(self.change_status)
-            self.open_u1.triggered.connect(self.restore_window)
-            self.open_u1_folder.triggered.connect(self.open_u1_folder_action)
-            self.get_more_storage.triggered.connect(
-                self.get_more_storage_action)
-            self.go_to_web.triggered.connect(self.go_to_web_action)
-            self.get_help_online.triggered.connect(self.get_help_action)
-            self.quit.triggered.connect(self.stop)
-
-            self._get_volumes_info()
+        # Items
+        self.context_menu.clear()
+
+        self.status = self.context_menu.addAction(LOADING)
+        self.status.setEnabled(False)
+        self.status_action = self.context_menu.addAction(PLEASE_WAIT)
+        self.refresh_status()
+        self.context_menu.addSeparator()
+
+        self.open_u1 = self.context_menu.addAction(OPEN_UBUNTU_ONE)
+        self.share_a_file = self.context_menu.addAction(SHARE_A_FILE)
+        self.open_u1_folder = self.context_menu.addAction(
+            OPEN_UBUNTU_ONE_FOLDER)
+        self.go_to_web = self.context_menu.addAction(GO_TO_WEB)
+        self.context_menu.addSeparator()
+
+        # Transfers
+        self.transfers = TransfersMenu(self)
+        self.context_menu.addMenu(self.transfers)
+
+        self.get_more_storage = self.context_menu.addAction(
+            GET_MORE_STORAGE)
+        self.get_help_online = self.context_menu.addAction(GET_HELP_ONLINE)
+        self.quit = self.context_menu.addAction("Quit")
+
+        self.setContextMenu(self.context_menu)
+
+        # Connect Signals
+        self.status_action.triggered.connect(self.change_status)
+        self.open_u1.triggered.connect(self.restore_window)
+        self.share_a_file.triggered.connect(self.open_share_tab)
+        self.open_u1_folder.triggered.connect(self.open_u1_folder_action)
+        self.get_more_storage.triggered.connect(
+            self.get_more_storage_action)
+        self.go_to_web.triggered.connect(self.go_to_web_action)
+        self.get_help_online.triggered.connect(self.get_help_action)
+        self.quit.triggered.connect(self.stop)
+
+        self._get_volumes_info()
 
     @inlineCallbacks
     def refresh_status(self):
@@ -278,6 +252,17 @@
         self.window.show()
         self.window.activateWindow()
 
+    def open_share_tab(self):
+        """Show the main window in the share tab."""
+        if self.window is None:
+            # pylint: disable=W0404
+            from ubuntuone.controlpanel.gui.qt.gui import MainWindow
+            # pylint: enable=W0404
+            self.window = MainWindow(close_callback=self.delete_window)
+        self.window.switch_to('share_links')
+        self.window.show()
+        self.window.activateWindow()
+
     def delete_window(self):
         """Close and remove the main window."""
         if self.window is not None:

=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_systray.py'
--- ubuntuone/controlpanel/gui/qt/tests/test_systray.py	2012-09-05 22:58:33 +0000
+++ ubuntuone/controlpanel/gui/qt/tests/test_systray.py	2012-10-29 19:38:20 +0000
@@ -60,6 +60,11 @@
     def __init__(self, *args, **kwargs):
         self.args = (args, kwargs)
         super(FakeMainWindow, self).__init__()
+        self.tabname = ''
+
+    def switch_to(self, tabname):
+        """Fake switch_to."""
+        self.tabname = tabname
 
 
 class SystrayTestCase(BaseTestCase):
@@ -72,7 +77,6 @@
     def setUp(self):
         # We need to patch the startTimer first, to avoid the timer
         # to get started on initialization.
-        self.patch(systray.TrayIcon, "startTimer", lambda s, x: None)
         self.patch(systray.TransfersMenu, "startTimer", lambda s, x: None)
         yield super(SystrayTestCase, self).setUp()
         self.fake_desktop_service = FakeDesktopService()
@@ -249,6 +253,46 @@
         tray.open_u1.trigger()
         self.assertEqual(self._called, ((), {}))
 
+    def test_open_share_file_no_window(self):
+        """Test the open_share_file option in the menu, with no window."""
+        self.patch(systray.TransfersMenu, "startTimer", lambda s, x: None)
+        self.patch(ubuntuone.controlpanel.gui.qt.gui,
+            "MainWindow", FakeMainWindow)
+        tray = systray.TrayIcon()
+        self.assertEqual(tray.window, None)
+        tray.share_a_file.trigger()
+        self.assertIsInstance(tray.window, FakeMainWindow)
+        self.assertTrue(tray.window.isVisible())
+        self.assertEqual(tray.window.tabname, 'share_links')
+        self.assertEqual(tray.window.args, ((),
+            {'close_callback': tray.delete_window}))
+
+    def test_open_share_file(self):
+        """Test the open_share_file option in the menu, with a window."""
+        self.patch(systray.TransfersMenu, "startTimer", lambda s, x: None)
+        tray = systray.TrayIcon()
+        window = FakeMainWindow()
+        tray.window = window
+        self.assertFalse(tray.window.isVisible())
+        tray.share_a_file.trigger()
+        self.assertEqual(tray.window, window)
+        self.assertEqual(tray.window.tabname, 'share_links')
+        self.assertTrue(tray.window.isVisible())
+
+    def test_open_share_file_window_minimized(self):
+        """Test the open_share_file option in the menu, with a min. window."""
+        self.patch(systray.TransfersMenu, "startTimer", lambda s, x: None)
+        tray = systray.TrayIcon()
+        window = FakeMainWindow()
+        # This cannot be tested with the real activateWindow
+        # because the un-minimization is done by the WM, so
+        # it has a small delay, and it fails.
+        self.patch(window, "activateWindow", self._set_called)
+        tray.window = window
+        tray.share_a_file.trigger()
+        self.assertEqual(tray.window.tabname, 'share_links')
+        self.assertEqual(self._called, ((), {}))
+
     def test_delete_window(self):
         """Test deleting an existing window."""
         self.patch(systray.TransfersMenu, "startTimer", lambda s, x: None)
@@ -337,7 +381,6 @@
     def setUp(self):
         # We need to patch the startTimer first, to avoid the timer
         # to get started on initialization.
-        self.patch(systray.TrayIcon, "startTimer", lambda s, x: None)
         self.patch(systray.TransfersMenu, "startTimer", lambda s, x: None)
         yield super(TransfersMenuTestCase, self).setUp()
         self.fake_desktop_service = FakeDesktopService()
@@ -531,58 +574,3 @@
         current_actions = self.ui.transfers.actions()
 
         self.assertNotEqual(previous_actions, current_actions)
-
-
-class SharesTestCase(BaseTestCase):
-
-    """Test the info to be displayed in the shares section."""
-
-    class_ui = systray.TrayIcon
-
-    @inlineCallbacks
-    def setUp(self):
-        # We need to patch the startTimer first, to avoid the timer
-        # to get started on initialization.
-        self.patch(systray.TrayIcon, "startTimer", lambda s, x: None)
-        self.patch(systray.TransfersMenu, "startTimer", lambda s, x: None)
-        yield super(SharesTestCase, self).setUp()
-        self.fake_desktop_service = FakeDesktopService()
-        self.patch(QtGui, "QDesktopServices", self.fake_desktop_service)
-        self.patch(self.ui.backend, "get_shares", self.fake_get_backend)
-        self._shares_info = []
-
-    def fake_get_backend(self):
-        """Fake get_backend."""
-        return self._shares_info
-
-    def test_shares_refresh_not_reload(self):
-        """Check that we get the new info of shares but not reload the menu."""
-        actions = self.ui.context_menu.actions()
-        self.ui.load_menu()
-        post_actions = self.ui.context_menu.actions()
-        self.assertEqual(actions, post_actions)
-
-    def test_shares_refresh_reload(self):
-        """Check that we get the new info of shares but not reload the menu."""
-        actions = self.ui.context_menu.actions()
-        self._shares_info = [
-            {'accepted': False, 'other_visible_name': 'name',
-            'volume_id': 'asd123-asd123-asd123-asd123'}
-        ]
-        self.ui.load_menu()
-        post_actions = self.ui.context_menu.actions()
-        self.assertNotEqual(actions, post_actions)
-
-    def test_share_triggered(self):
-        """Check that we get the new info of shares but not reload the menu."""
-        volume_id = 'asd123-asd123-asd123-asd123'
-        self._shares_info = [
-            {'accepted': False, 'other_visible_name': 'name',
-            'volume_id': volume_id}
-        ]
-        self.ui.load_menu()
-        actions = self.ui.context_menu.actions()
-        share_action = actions[7]
-        share_action.trigger()
-        expected = QtCore.QUrl(systray.ACCEPT_SHARES % volume_id)
-        self.assertEqual(self.fake_desktop_service.opened_url, expected)