Merge lp:~nataliabidart/software-center/cant-stop-the-music into lp:software-center/5.2

[Natalia Bidart]

- Stop the video if user navigates away from an app details page
  (LP: #1003954).

[Michael Vogt (mvo)]

Thanks Natalia, this is great work!

There is one small issue:
- navigate to "plutoids"
- click on "back"
- click on "forward"
- the video frame is now blank because on navigation to the page the url is not updated
  (as the view is "clever" and remembers that it does not need to reload the page)

It would be great if you could have a look at this.

Another thought for trunk would be to decouple the "stop" from the ViewManager.display_page and add
signals or functions like "enter-page", "leave-page" so that the page itself can do the magic that needs
to be done (like stop video or store the vadjustment in the applist). What do you think about
this?

[Michael Vogt (mvo)]

(oh, and I love your branch names)

[Natalia Bidart (nataliabidart)]

> Thanks Natalia, this is great work!
>
> There is one small issue:
> - navigate to "plutoids"
> - click on "back"
> - click on "forward"
> - the video frame is now blank because on navigation to the page the url is
> not updated
> (as the view is "clever" and remembers that it does not need to reload the
> page)
>
> It would be great if you could have a look at this.

Nice catch! Will be fixing this ASAP.

> Another thought for trunk would be to decouple the "stop" from the
> ViewManager.display_page and add
> signals or functions like "enter-page", "leave-page" so that the page itself
> can do the magic that needs
> to be done (like stop video or store the vadjustment in the applist). What do
> you think about
> this?

I would love that considering that the current proposed solution couples the different modules too much.

You want me to propose a branch against trunk? is it ok if trunk and 5.2 start diverging?

[Michael Vogt (mvo)]

On Mon, Jun 18, 2012 at 11:51:34AM -0000, Natalia Bidart wrote:
> > Thanks Natalia, this is great work!
> >
> > There is one small issue:
> > - navigate to "plutoids"
> > - click on "back"
> > - click on "forward"
> > - the video frame is now blank because on navigation to the page the url is
> > not updated
> > (as the view is "clever" and remembers that it does not need to reload the
> > page)
> >
> > It would be great if you could have a look at this.
>
> Nice catch! Will be fixing this ASAP.
Thanks!

> > Another thought for trunk would be to decouple the "stop" from the
> > ViewManager.display_page and add
> > signals or functions like "enter-page", "leave-page" so that the page itself
> > can do the magic that needs
> > to be done (like stop video or store the vadjustment in the applist). What do
> > you think about
> > this?
>
> I would love that considering that the current proposed solution couples the different modules too much.
>
> You want me to propose a branch against trunk? is it ok if trunk and 5.2 start diverging?

I think for 5.2 this branch is perfect as it makes the SRU upload
easier (small amount of code changes etc). For the improved fix we
should target trunk and I think its ok if the two branches diverge,
its a unavoidable in the longer term anyway :)

Cheers,
 Michael

[lp:~nataliabidart/software-center/cant-stop-the-music]

3053. By Natalia Bidart on 2012-06-18

Ensure that video gets updated when navigating back to an already visited app.

[Michael Vogt (mvo)]

Thanks for the fix, this looks fine now. Plus I appreciate the cleanup in the test, e.g. the new
 "set_mock_app_and_details()"

review: Approve

=== modified file 'softwarecenter/ui/gtk3/session/navhistory.py'
--- softwarecenter/ui/gtk3/session/navhistory.py	2012-03-15 00:16:03 +0000
+++ softwarecenter/ui/gtk3/session/navhistory.py	2012-06-18 14:42:18 +0000
@@ -124,9 +124,8 @@
 
     def __str__(self):
         facet = self.pane.pane_name.replace(' ', '')[:6]
-        return utf8("%s:%s %s") % (facet,
-                                   self.page,
-                                   self.view_state)
+        result = utf8("%s:%s %s") % (facet, self.page, self.view_state)
+        return result
 
     def navigate_to(self):
         """

=== modified file 'softwarecenter/ui/gtk3/session/viewmanager.py'
--- softwarecenter/ui/gtk3/session/viewmanager.py	2012-04-16 21:51:02 +0000
+++ softwarecenter/ui/gtk3/session/viewmanager.py	2012-06-18 14:42:18 +0000
@@ -177,6 +177,11 @@
             if ni.pane.is_applist_view_showing():
                 v = ni.pane.app_view.tree_view_scroll.get_vadjustment()
                 ni.view_state.vadjustment = v.get_value()
+            # if there is a video playing, stop it
+            try:
+                ni.pane.app_details_view.videoplayer.stop()
+            except AttributeError:
+                pass
 
         if callback is None:
             callback = pane.get_callback_for_page(page, view_state)

=== modified file 'softwarecenter/ui/gtk3/views/appdetailsview.py'
--- softwarecenter/ui/gtk3/views/appdetailsview.py	2012-05-22 07:29:34 +0000
+++ softwarecenter/ui/gtk3/views/appdetailsview.py	2012-06-18 14:42:18 +0000
@@ -1565,6 +1565,7 @@
         self._update_pkg_info_table(app_details)
         self._update_warning_bar(app_details)
 #        self._update_addons_minimal(app_details)
+        self._update_app_video(app_details)
 
         # depending on pkg install state set action labels
         self.pkg_statusbar.configure(app_details, app_details.pkg_state)
@@ -2042,7 +2043,7 @@
         self.section = section
 
 
-def get_test_window_appdetails():
+def get_test_window_appdetails(pkgname="totem"):
 
     from softwarecenter.db.pkginfo import get_pkg_info
     cache = get_pkg_info()
@@ -2068,12 +2069,6 @@
     scroll = Gtk.ScrolledWindow()
     view = AppDetailsView(db, distro, icons, cache, datadir)
 
-    import sys
-    if len(sys.argv) > 1:
-        pkgname = sys.argv[1]
-    else:
-        pkgname = "totem"
-
     view.show_app(Application("", pkgname))
     #view.show_app(Application("Pay App Example", "pay-app"))
     #view.show_app(Application("3D Chess", "3dchess"))
@@ -2105,7 +2100,13 @@
             view.show_app(Application("Movie Player", "totem"))
         return True
 
-    win = get_test_window_appdetails()
+    import sys
+    if len(sys.argv) > 1:
+        pkgname = sys.argv[1]
+    else:
+        pkgname = "totem"
+
+    win = get_test_window_appdetails(pkgname=pkgname)
 
     # keep it spinning to test for re-draw issues and memleaks
     #view = win.get_data("view")

=== modified file 'softwarecenter/ui/gtk3/widgets/videoplayer.py'
--- softwarecenter/ui/gtk3/widgets/videoplayer.py	2012-04-18 21:04:11 +0000
+++ softwarecenter/ui/gtk3/widgets/videoplayer.py	2012-06-18 14:42:18 +0000
@@ -91,6 +91,9 @@
         base_uri = "http://www.ubuntu.com"
         self.webkit.load_html_string(html, base_uri)
 
+    def stop(self):
+        self.webkit.load_uri('')
+
 
 # AKA the-segfault-edition-with-no-documentation
 class VideoPlayerGtk3(Gtk.VBox):

=== modified file 'test/gtk3/test_appdetailsview.py'
--- test/gtk3/test_appdetailsview.py	2012-04-17 14:27:43 +0000
+++ test/gtk3/test_appdetailsview.py	2012-06-18 14:42:18 +0000
@@ -20,10 +20,11 @@
 from test.test_database import make_purchased_app_details
 
 
-
 # window destory timeout
 TIMEOUT=100
 
+
+
 class TestAppdetailsView(unittest.TestCase):
 
     def setUp(self):
@@ -34,6 +35,16 @@
         GObject.timeout_add(TIMEOUT, lambda: self.win.destroy())
         Gtk.main()
 
+    def set_mock_app_and_details(self, app_name="software-center", **kwargs):
+        app = Application("", app_name)
+        mock_app = get_mock_app_from_real_app(app)
+        mock_details = mock_app.get_details(None)
+        for attr, value in kwargs.iteritems():
+            setattr(mock_details, attr, value)
+
+        self.view.app = mock_app
+        self.view.app_details = mock_details
+
     def test_videoplayer(self):
         # show app with no video
         app = Application("", "2vcard")
@@ -42,34 +53,30 @@
         self.assertFalse(self.view.videoplayer.get_property("visible"))
 
         # create app with video and ensure its visible
-        app = Application("", "synaptic")
-        mock = get_mock_app_from_real_app(app)
-        details = mock.get_details(None)
-        # this is a example html - any html5 video will do
-        details.video_url = "http://people.canonical.com/~mvo/totem.html"
-        self.view.show_app(mock)
+        self.set_mock_app_and_details(
+            app_name="synaptic",
+            # this is a example html - any html5 video will do
+            video_url="http://people.canonical.com/~mvo/totem.html")
+        self.view.show_app(self.view.app)
         do_events()
         self.assertTrue(self.view.videoplayer.get_property("visible"))
-    
+
     def test_page_pkgstates(self):
-        # show app 
+        # show app
         app = Application("", "abiword")
         self.view.show_app(app)
         do_events()
-        
+
         # check that the action bar is given initial focus in the view
         self.assertTrue(self.view.pkg_statusbar.button.is_focus())
 
         # create mock app
-        mock_app = get_mock_app_from_real_app(app)
-        self.view.app = mock_app
-        mock_details = mock_app.get_details(None)
-        mock_details.purchase_date = "2011-11-20 17:45:01"
-        mock_details._error_not_found = "error not found"
-        mock_details.price = "1.00"
-        mock_details.pkgname = "abiword"
-        mock_details.error = "error-text"
-        self.view.app_details = mock_details
+        self.set_mock_app_and_details(
+            app_name="abiword", purchase_date="2011-11-20 17:45:01",
+            _error_not_found="error not found", price="1.00",
+            pkgname="abiword", error="error-text")
+        mock_app = self.view.app
+        mock_details = self.view.app_details
 
         # the states and what labels we expect in the pkgstatusbar
         # first string is status text, second is button text
@@ -101,7 +108,7 @@
             if state in pkg_states_to_labels:
                 label, button_label = pkg_states_to_labels[state]
                 self.assertEqual(
-                    self.view.pkg_statusbar.get_label(), 
+                    self.view.pkg_statusbar.get_label(),
                     label.decode("utf-8"))
                 self.assertEqual(
                     self.view.pkg_statusbar.get_button_label().decode("utf-8"),
@@ -122,15 +129,10 @@
 
     def test_app_icon_loading(self):
         # get icon
-        app = Application("", "software-center")
-        mock_app = get_mock_app_from_real_app(app)
-        self.view.app = mock_app
-        mock_details = mock_app.get_details(None)
-        mock_details.cached_icon_file_path = "download-icon-test"
-        mock_details.icon = "favicon.ico"
-        mock_details.icon_url = "http://de.wikipedia.org/favicon.ico"
-        self.view.app_details = mock_details
-        self.view.show_app(mock_app)
+        self.set_mock_app_and_details(
+            cached_icon_file_path="download-icon-test", icon="favicon.ico",
+            icon_url="http://de.wikipedia.org/favicon.ico")
+        self.view.show_app(self.view.app)
         do_events()
         # ensure the icon is there
         # FIXME: ensure that the icon is really downloaded
@@ -250,10 +252,21 @@
         self.view._submit_reviews_done_callback(None, 0)
 
         self.assertTrue(button.is_sensitive())
-        
+
+    def test_show_app_twice_plays_video(self):
+        video_url = "http://people.canonical.com/~mvo/totem.html"
+        self.set_mock_app_and_details(video_url=video_url)
+
+        self.view.show_app(self.view.app)
+        self.assertEqual(self.view.videoplayer.uri, video_url)
+
+        self.view.videoplayer.uri = None
+        self.view.show_app(self.view.app)
+        self.assertEqual(self.view.videoplayer.uri, video_url)
+
 
 class MultipleVersionsTestCase(unittest.TestCase):
-    
+
     @classmethod
     def setUpClass(cls):
         # Set these as class attributes as we don't modify either
@@ -278,7 +291,7 @@
         self.view.show_app(self.app_mock)
         self.assertFalse(self.view.pkg_statusbar.combo_multiple_versions.get_visible())
         # switch to not-automatic app with different description
-        self.app_mock.details.get_not_automatic_archive_versions = lambda: [ 
+        self.app_mock.details.get_not_automatic_archive_versions = lambda: [
             ("5.0", "precise"),
             ("12.0", "precise-backports"),
             ]
@@ -290,7 +303,7 @@
     def test_combo_multiple_versions(self):
         self.app_mock.details.get_not_automatic_archive_versions = lambda: [
             ("5.0",  "precise"),
-            ("12.0", "precise-backports") 
+            ("12.0", "precise-backports")
             ]
         # ensure that the right method is called
         self.app_mock.details.force_not_automatic_archive_suite = Mock()
@@ -305,7 +318,7 @@
     def test_installed_multiple_version_default(self):
         self.app_mock.details.get_not_automatic_archive_versions = lambda: [
             ("5.0",  "precise"),
-            ("12.0", "precise-backports") 
+            ("12.0", "precise-backports")
             ]
         self.app_mock.details.pkg_state = PkgStates.INSTALLED
         self.app_mock.details.version = "12.0"
@@ -332,10 +345,10 @@
         #self.assertEqual(call_args, (("precise",), {}))
         # ensure the button changes
         self.assertEqual(self.view.pkg_statusbar.button.get_label(), "Change")
-        
+
 
 class HardwareRequirementsTestCase(unittest.TestCase):
-    
+
     @classmethod
     def setUpClass(cls):
         # Set these as class attributes as we don't modify either
@@ -356,7 +369,7 @@
         self.app_mock.details.pkg_state = PkgStates.UNINSTALLED
 
     def test_show_hardware_requirements(self):
-        self.app_mock.details.hardware_requirements = { 
+        self.app_mock.details.hardware_requirements = {
             'hardware::video:opengl' : 'yes',
             'hardware::gps' : 'no',
             }
@@ -378,7 +391,7 @@
         self.app_mock.details.pkg_state = PkgStates.NEEDS_PURCHASE
         self.view.show_app(self.app_mock)
         self.assertEqual(
-            self.view.pkg_statusbar.button.get_label(), 
+            self.view.pkg_statusbar.button.get_label(),
             _(u"Buy Anyway\u2026").encode("utf-8"))
         # check if the warning bar is displayed
         self.assertTrue(self.view.pkg_warningbar.get_property("visible"))
@@ -401,13 +414,13 @@
         self.app_mock.details.pkg_state = PkgStates.NEEDS_PURCHASE
         self.view.show_app(self.app_mock)
         self.assertEqual(
-            self.view.pkg_statusbar.button.get_label(), 
+            self.view.pkg_statusbar.button.get_label(),
             _(u'Buy\u2026').encode("utf-8"))
         # check if the warning bar is invisible
         self.assertFalse(self.view.pkg_warningbar.get_property("visible"))
 
 class RegionRequirementsTestCase(unittest.TestCase):
-    
+
     @classmethod
     def setUpClass(cls):
         # Set these as class attributes as we don't modify either
@@ -438,7 +451,7 @@
         self.app_mock.details.pkg_state = PkgStates.NEEDS_PURCHASE
         self.view.show_app(self.app_mock)
         self.assertEqual(
-            self.view.pkg_statusbar.button.get_label(), 
+            self.view.pkg_statusbar.button.get_label(),
             _(u"Buy Anyway\u2026").encode("utf-8"))
         # check if the warning bar is displayed
         self.assertTrue(self.view.pkg_warningbar.get_property("visible"))
@@ -537,9 +550,9 @@
             self.assertEqual(
                 [method_name],
                 all_method_calls)
-                
+
 class AppRecommendationsTestCase(unittest.TestCase):
-    
+
     @classmethod
     def setUpClass(cls):
         # Set these as class attributes as we don't modify either
@@ -558,11 +571,11 @@
         app = Application("", "pitivi")
         self.app_mock = get_mock_app_from_real_app(app)
         self.app_mock.details.pkg_state = PkgStates.UNINSTALLED
-        
+
     def on_query_done(self, recagent, data):
         print "query done, data: '%s'" % data
         self.loop.quit()
-        
+
     def on_query_error(self, recagent, error):
         print "query error received: ", error
         self.loop.quit()

=== modified file 'test/gtk3/test_appmanager.py'
--- test/gtk3/test_appmanager.py	2012-01-16 14:42:49 +0000
+++ test/gtk3/test_appmanager.py	2012-06-18 14:42:18 +0000
@@ -14,6 +14,7 @@
 from softwarecenter.ui.gtk3.session.appmanager import (
     ApplicationManager, get_appmanager)
 
+
 class TestAppManager(unittest.TestCase):
     """ tests the appmanager  """
 
@@ -38,7 +39,7 @@
         # test creating it twice raises a error
         self.assertRaises(
             ValueError, ApplicationManager, self.db, self.backend, self.icons)
-        
+
     def test_appmanager(self):
         app_manager = get_appmanager()
         self.assertNotEqual(app_manager, None)

=== added file 'test/gtk3/test_viewmanager.py'
--- test/gtk3/test_viewmanager.py	1970-01-01 00:00:00 +0000
+++ test/gtk3/test_viewmanager.py	2012-06-18 14:42:18 +0000
@@ -0,0 +1,59 @@
+#!/usr/bin/python
+
+import unittest
+
+from gi.repository import Gtk
+from mock import Mock
+
+from testutils import setup_test_env
+setup_test_env()
+
+from softwarecenter.ui.gtk3.panes import availablepane, softwarepane
+from softwarecenter.ui.gtk3.session.viewmanager import (
+    ViewManager,
+    get_viewmanager
+)
+
+
+class TestViewManager(unittest.TestCase):
+    """Test suite for the viewmanager."""
+
+    def setUp(self):
+        # create it once, it becomes global instance
+        self.vm = get_viewmanager()
+        if self.vm is None:
+            self.vm = ViewManager(Gtk.Notebook())
+
+        self.addCleanup(self.vm.destroy)
+
+    def test_get_viewmanager(self):
+        view_manager = get_viewmanager()
+        self.assertNotEqual(view_manager, None)
+        # is a singleton singleton
+        view_manager2 = get_viewmanager()
+        self.assertIs(view_manager, view_manager2)
+        # test creating it twice raises a error
+        self.assertRaises(ValueError, ViewManager, Gtk.Notebook())
+
+    def test_display_page_stops_video(self):
+        called = []
+        # navigate to an app details view with video
+        pane = Mock()
+        pane.pane_name = 'MockPane'
+        pane.is_applist_view_showing = lambda: False
+        pane.app_details_view.videoplayer.stop = lambda: called.append('stop')
+        self.vm.display_page(
+            pane, page=availablepane.AvailablePane.Pages.DETAILS,
+            view_state=softwarepane.DisplayState())
+
+        other_pane = Mock()
+        other_pane.pane_name = 'MockPane'
+        self.vm.display_page(
+            other_pane, page=availablepane.AvailablePane.Pages.DETAILS,
+            view_state=softwarepane.DisplayState())
+
+        self.assertEqual(called, ['stop'])
+
+
+if __name__ == "__main__":
+    unittest.main()

=== modified file 'test/gtk3/test_widgets.py'
--- test/gtk3/test_widgets.py	2012-04-12 07:46:56 +0000
+++ test/gtk3/test_widgets.py	2012-06-18 14:42:18 +0000
@@ -10,6 +10,7 @@
 from testutils import setup_test_env, do_events
 setup_test_env()
 from softwarecenter.utils import utf8
+from softwarecenter.ui.gtk3.widgets import videoplayer
 from softwarecenter.ui.gtk3.widgets.reviews import get_test_reviews_window
 from softwarecenter.ui.gtk3.widgets.labels import (
     HardwareRequirementsLabel, HardwareRequirementsBox)
@@ -118,13 +119,13 @@
         GObject.timeout_add(TIMEOUT, lambda: win.destroy())
         Gtk.main()
 
-
     def test_apptreeview(self):
         from softwarecenter.ui.gtk3.widgets.apptreeview import get_test_window
         win = get_test_window()
         GObject.timeout_add(TIMEOUT, lambda: win.destroy())
         Gtk.main()
 
+
 class TestHWRequirements(unittest.TestCase):
 
     HW_TEST_RESULT = { 'hardware::gps' : 'yes',
@@ -177,7 +178,7 @@
         # test seting it again
         box.set_hardware_requirements(self.HW_TEST_RESULT)
         self.assertEqual(len(box.get_children()), 2)
-        
+
 
 class TestUIReviewsList(unittest.TestCase):
     def setUp(self):
@@ -241,6 +242,30 @@
             _('Be the first to contribute a review for this application'))
 
 
+class VideoPlayerTestCase(unittest.TestCase):
+
+    def setUp(self):
+        super(VideoPlayerTestCase, self).setUp()
+        self.webkit_uri = None
+        self.vp = videoplayer.VideoPlayer()
+        self.vp.webkit.load_uri = lambda uri: setattr(self, 'webkit_uri', uri)
+        self.vp.webkit.get_uri = lambda: self.webkit_uri
+        self.addCleanup(self.vp.destroy)
+
+    def test_uri(self):
+        self.assertEqual(self.vp.uri, '')
+
+        expected_uri = 'file://test'
+        self.vp.uri = expected_uri
+
+        self.assertEqual(self.vp.uri, expected_uri)
+        self.assertEqual(self.vp.webkit.get_uri(), self.vp.uri)
+
+    def test_stop(self):
+        self.vp.uri = 'http://foo.bar.baz'
+        self.vp.stop()
+
+        self.assertEqual(self.vp.webkit.get_uri(), '')
 
 
 if __name__ == "__main__":