Merge lp:~dyams/unity-2d/launcher-update-visible-menu into lp:unity-2d

[Lohith D Shivamurthy]

[launcher] A visible context menu should be updated properly while application is closing or launching

'Remove from launcher' action is displayed when application is not running.
'Keep in launcher' & 'Quit' actions are displayed while application is running.

Rest of the context actions remain unchanged.

[Gerry Boland (gerboland)]

It's possible to click a slow-starting application tile, right-click and select "Quit" all before the application starts up. Then it does not quit.

Please check how Unity behaves.

review: Needs Fixing

[Lohith D Shivamurthy (dyams)]

> It's possible to click a slow-starting application tile, right-click and
> select "Quit" all before the application starts up. Then it does not quit.
>

Agree with you. Was over-conscious :)

Actually this branch has lot of conflicts with the trunk already.
Hence I have created a new branch.

Would it be possible to accept this new branch?

https://code.launchpad.net/~dyams/unity-2d/update-launcher-context-menu

Thank you.

After your answer, I remove this branch or remove this link from the bug definition.

[lp:~dyams/unity-2d/launcher-update-visible-menu]

788. By Lohith D Shivamurthy on 2011-11-25

Do not update context menu while application is launching

[Lohith D Shivamurthy (dyams)]

> After your answer, I remove this branch or remove this link from the bug
> definition.

I removed my new branch. And updated this same branch. Please have a look :)

[lp:~dyams/unity-2d/launcher-update-visible-menu]

789. By Lohith D Shivamurthy on 2012-01-26

[launcher] merge with trunk

790. By Lohith D Shivamurthy on 2012-01-26

[launcher] test added

791. By Lohith D Shivamurthy on 2012-01-26

[launcher] added missing test file

[Albert Astals Cid (aacid)]

Things that need fixing:
 * You should disconnect the triggered signal, otherwise that menu entry is not connected to any slot any more.
 * The test file copyright should be on your name not on mine
 * The test objectives and steps are there from c&p and not updated to what your test really does

Thing to discuss:
 * In updateVisibleContextMenu it might be a better idea to compare using the action object names than the action texts.

review: Needs Fixing

[Albert Astals Cid (aacid)]

Something else i forgot, I am not sure why you are doing
   if (action->parent() == m_menu) {
could you clarify? Is it in case it is not the action we created? If you are removing it from the menu you might as well delete it too. Even better, if you change to checking the objectname as i suggested, you'll be "almost really sure" it is the action we created, so there is no need to do this check.

review: Needs Fixing

[Lohith D Shivamurthy (dyams)]

> Things that need fixing:
> * You should disconnect the triggered signal, otherwise that menu entry is
> not connected to any slot any more.

As I can see I have disconnected the 'onKeepTriggered' in the same update method. Did you mean any other signal/slot?

> * The test file copyright should be on your name not on mine
> * The test objectives and steps are there from c&p and not updated to what
> your test really does
>
Done
> Thing to discuss:
> * In updateVisibleContextMenu it might be a better idea to compare using the
> action object names than the action texts.
Agreed, but comparing unicode strings shouldn't harm either.

[Lohith D Shivamurthy (dyams)]

> Something else i forgot, I am not sure why you are doing
> if (action->parent() == m_menu) {
> could you clarify? Is it in case it is not the action we created? If you are
> removing it from the menu you might as well delete it too. Even better, if you
> change to checking the objectname as i suggested, you'll be "almost really
> sure" it is the action we created, so there is no need to do this check.
Yes, that was not necessary, but am sure i'm deleting it as soon as removing it from the menu.
action->deleteLater is there. no? Or do you mean something else?
Yes, am using object names now.

[lp:~dyams/unity-2d/launcher-update-visible-menu]

792. By Lohith D Shivamurthy on 2012-01-27

[launcher] Using object names to identify the QAction. Also fixed the tests

[Albert Astals Cid (aacid)]

> > Things that need fixing:
> > * You should disconnect the triggered signal, otherwise that menu entry is
> > not connected to any slot any more.
>
> As I can see I have disconnected the 'onKeepTriggered' in the same update
> method. Did you mean any other signal/slot?

What I am saying is that you should not disconnect the signal. If you do when you click the "Remove from launcher" action it will do nothing.

review: Needs Fixing

[lp:~dyams/unity-2d/launcher-update-visible-menu]

793. By Lohith D Shivamurthy on 2012-01-27

[launcher] make 'Remove from launcher' action uncheckable while application is closed and leave the previous connection as it is

[Albert Astals Cid (aacid)]

There is a problem with the test, for it to work you need to have gedit in the launcher which i don't, so the test does not work here since when the gedit is closed the tile disappears from the launcher.

I suggest you to have a look at tests/launcher/autohide_show_tests.rb where we read/set the favorites, so on starting your test you set the favorites so it always includes gedit.

Also it would be nice that after checking that the 'Remove from launcher' is there, you check it works (i.e. click on it and then see the tile is not there) since that is what was not working when you had the disconnect there in previous versions of the code.

review: Needs Fixing

[lp:~dyams/unity-2d/launcher-update-visible-menu]

794. By Lohith D Shivamurthy on 2012-01-27

[launcher] fixed tests

[Albert Astals Cid (aacid)]

More test fixing sorry :D

My gedit opened in full screen and that made the launcher bar hide, make sure you move the mouse to the left of the screen and then wait for the launcher to show before trying to right click on the tile.

review: Needs Fixing

[lp:~dyams/unity-2d/launcher-update-visible-menu]

795. By Lohith D Shivamurthy on 2012-01-27

[launcher] fix tests to verify 'Remove from launcher' remove the tile from launcher

[Albert Astals Cid (aacid)]

I still need this
+ XDo::Mouse.move(0, y, 0, true)
+ verify_equal( 0, TIMEOUT, 'Launcher hiding with mouse on the edge, should be visible' ) {
+ @app.Unity2dPanel()['x_absolute'].to_i
+ }
before
XDo::Mouse.click(0, y, :right, 40)
to make sure the launcher is showing

review: Needs Fixing

[lp:~dyams/unity-2d/launcher-update-visible-menu]

796. By Lohith D Shivamurthy on 2012-01-28

[launcher] make sure launcher is visible before opening the contextual menu

[Albert Astals Cid (aacid)]

One last minor thing, i think that
  'Could not find the test application tile in the launcher'
should be something like
  'Could find the test application tile in the launcher, should not be there'

[Albert Astals Cid (aacid)]

As per previous comment, setting "Needs Fixing"

review: Needs Fixing

[lp:~dyams/unity-2d/launcher-update-visible-menu]

797. By Lohith D Shivamurthy on 2012-01-31

[launcher] Fix verify display comments

[Albert Astals Cid (aacid)]

I just realized there is one thing missing in this patch. It is handling the situation in which you have the application running, the menu open and then the application closes, but it is not handling the situation in which you do not have the application running, the menu is open and then the application starts, you need to update the menu in that situation too, right?

review: Needs Fixing

[Lohith D Shivamurthy (dyams)]

> I just realized there is one thing missing in this patch. It is handling the
> situation in which you have the application running, the menu open and then
> the application closes, but it is not handling the situation in which you do
> not have the application running, the menu is open and then the application
> starts, you need to update the menu in that situation too, right?
no. As you can see in the beginning of this page, I intentionally removed that code.

[Albert Astals Cid (aacid)]

But then the menu is wrong, it still says "Remove from Launcher" when it should say "Keep in Launcher", right?

[Lohith D Shivamurthy (dyams)]

> But then the menu is wrong, it still says "Remove from Launcher" when it
> should say "Keep in Launcher", right?
Yes, I have put this comment in the bug too : https://bugs.launchpad.net/unity-2d/+bug/784541/comments/1

[Albert Astals Cid (aacid)]

Doesn't make much sense to me fixing one part of the bug but not the other.

Gerry could you clarify if fixing only half of the bug is indeed enough/what we want?

[Gerry Boland (gerboland)]

I must side with Albert here, it does not make sense fixing one half of the bug & not the other. I would prefer if you took a little more time & perform a comprehensive fix.

Note that with the feature freeze approaching, I want to de-prioritise this task until after the freeze. So please continue with your current task.
-G

review: Needs Fixing

[Gerry Boland (gerboland)]

Branch abandoned, so rejecting this MR.

review: Disapprove

Unmerged revisions

797. By Lohith D Shivamurthy on 2012-01-31

[launcher] Fix verify display comments

796. By Lohith D Shivamurthy on 2012-01-28

[launcher] make sure launcher is visible before opening the contextual menu

795. By Lohith D Shivamurthy on 2012-01-27

[launcher] fix tests to verify 'Remove from launcher' remove the tile from launcher

794. By Lohith D Shivamurthy on 2012-01-27

[launcher] fixed tests

793. By Lohith D Shivamurthy on 2012-01-27

[launcher] make 'Remove from launcher' action uncheckable while application is closed and leave the previous connection as it is

792. By Lohith D Shivamurthy on 2012-01-27

[launcher] Using object names to identify the QAction. Also fixed the tests

791. By Lohith D Shivamurthy on 2012-01-26

[launcher] added missing test file

790. By Lohith D Shivamurthy on 2012-01-26

[launcher] test added

789. By Lohith D Shivamurthy on 2012-01-26

[launcher] merge with trunk

788. By Lohith D Shivamurthy on 2011-11-25

Do not update context menu while application is launching

=== modified file 'libunity-2d-private/src/launcherapplication.cpp'
--- libunity-2d-private/src/launcherapplication.cpp	2012-01-18 11:50:32 +0000
+++ libunity-2d-private/src/launcherapplication.cpp	2012-01-31 09:42:25 +0000
@@ -452,6 +452,7 @@
     m_application = NULL;
     updateBamfApplicationDependentProperties();
     closed();
+    updateVisibleContextMenu();
 }
 
 void
@@ -629,6 +630,29 @@
 }
 
 void
+LauncherApplication::updateVisibleContextMenu()
+{
+    // There is no context menu while application is closing ? Do nothing.
+    if (m_menu && !m_menu->isVisible() && m_menu->folded())
+        return;
+
+    QList<QAction*> contextActions = m_menu->actions();
+
+    Q_FOREACH(QAction* action, contextActions) {
+        if (action->objectName() == QString("keepInLauncherAction")) {
+            action->setChecked(false);
+            action->setCheckable(false);
+            action->setText(u2dTr("Remove from launcher"));
+        }
+
+        if (action->objectName() == QString("quitAction")) {
+            m_menu->removeAction(action);
+            action->deleteLater();
+        }
+    }
+}
+
+void
 LauncherApplication::activate()
 {
     if (urgent()) {
@@ -902,6 +926,7 @@
         keep->setCheckable(is_running);
         keep->setChecked(sticky());
         keep->setText(is_running ? u2dTr("Keep in launcher") : u2dTr("Remove from launcher"));
+        keep->setObjectName("keepInLauncherAction");
         actions.append(keep);
         QObject::connect(keep, SIGNAL(triggered()), this, SLOT(onKeepTriggered()));
     }
@@ -909,6 +934,7 @@
     if (is_running) {
         QAction* quit = new QAction(m_menu);
         quit->setText(u2dTr("Quit"));
+        quit->setObjectName("quitAction");
         actions.append(quit);
         QObject::connect(quit, SIGNAL(triggered()), this, SLOT(onQuitTriggered()));
     }

=== modified file 'libunity-2d-private/src/launcherapplication.h'
--- libunity-2d-private/src/launcherapplication.h	2012-01-13 15:05:44 +0000
+++ libunity-2d-private/src/launcherapplication.h	2012-01-31 09:42:25 +0000
@@ -173,7 +173,7 @@
     template<typename T>
     bool updateOverlayState(const QMap<QString, QVariant>& properties,
                             const QString& propertyName, T* member);
-
+    void updateVisibleContextMenu();
     QString m_dynamicQuicklistPath;
     QScopedPointer<DBusMenuImporter> m_dynamicQuicklistImporter;
     QDBusServiceWatcher* m_dynamicQuicklistServiceWatcher;

=== added file 'tests/launcher/update-visible-menu.rb'
--- tests/launcher/update-visible-menu.rb	1970-01-01 00:00:00 +0000
+++ tests/launcher/update-visible-menu.rb	2012-01-31 09:42:25 +0000
@@ -0,0 +1,143 @@
+#!/usr/bin/env ruby1.8
+=begin
+/*
+ * This file is part of unity-2d
+ *
+ * Copyright 2012 Canonical Ltd.
+ *
+ * Authors:
+ * - Lohith DS <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+=end
+
+require '../run-tests.rb' unless $INIT_COMPLETED
+require 'xdo/xwindow'
+require 'xdo/keyboard'
+require 'xdo/mouse'
+
+############################# Test Suite #############################
+context "Launcher visible contextual menu Tests" do
+  # Run once at the beginning of this test suite
+  startup do
+    $SUT.execute_shell_command 'killall unity-2d-launcher'
+    $SUT.execute_shell_command 'killall unity-2d-launcher'
+
+    $SUT.execute_shell_command 'killall gedit'
+    # Minimize all windows
+    XDo::XWindow.toggle_minimize_all
+
+    # Make certain application is ready for testing
+    test_fav = $SUT.execute_shell_command 'gsettings get com.canonical.Unity.Launcher favorites'
+    if test_fav.index("gedit.desktop") == nil
+        test_fav.insert(1, "'gedit.desktop', ")
+        $SUT.execute_shell_command "gsettings set com.canonical.Unity.Launcher favorites \"" + test_fav + "\""
+    end
+  end
+
+  # Run once at the end of this test suite
+  shutdown do
+  end
+
+  # Run before each test case begins
+  setup do
+    # Execute the application
+    @app = $SUT.run( :name => UNITY_2D_LAUNCHER,
+                     :arguments => "-testability",
+                     :sleeptime => 2 )
+    verify{ @app.Launcher() }
+
+    $SUT.execute_shell_command('gedit', :detached => true)
+  end
+
+  # Run after each test case completes
+  teardown do
+    $SUT.execute_shell_command 'killall gedit'
+    $SUT.execute_shell_command 'pkill -nf unity-2d-launcher'
+  end
+
+  #####################################################################################
+  # Test cases
+
+  # Test case objectives:
+  #   * Launcher visible menu updates when app closed
+  # Pre-conditions
+  #   * close running gedit windows and launch a single gedit window
+  # Test steps
+  #   * Find the 'gedit' tile in the launcher
+  #   * Open contextual menu
+  #   * Verify that menu has 'Keep in launcher' action
+  #   * Close the window through Alt-F4
+  #   * Verify that the menu action 'Keep in launcher' is replaced with 'Remove from launcher'
+  #   * Verify that the action 'Quit' is removed from menu
+  #   * left click on 'Remove from launcher' action
+  #   * Verify that application tile is removed from the launcher
+  # Post-conditions
+  #   * None
+  # References
+  #   * lp:784541
+  test "Launcher visible menu updates when app closed" do
+    tiles = ""
+    verify( TIMEOUT, 'Could not find the test application tile in the launcher' ) {
+        tiles = @app.Launcher().LauncherList( :name => 'main' ).children( { :running => 'true', :name => 'gedit' } )
+    }
+
+    if !tiles.empty?
+        tile = tiles[0]
+
+        y = tile['y_absolute'].to_i + 20
+
+        XDo::Mouse.move(0, y, 0, true)
+        verify_equal( 0, TIMEOUT, 'Launcher hiding with mouse on the edge, should be visible' ) {
+            @app.Unity2dPanel()['x_absolute'].to_i
+        }
+
+        XDo::Mouse.click(0, y, :right, 40)
+
+        context_menu = ""
+        verify( TIMEOUT, 'Tile contextual menu not found' ) {
+            context_menu = @app.LauncherContextualMenu()
+        }
+
+        verify_equal('Keep in launcher', TIMEOUT, "'Keep in launcher' action was expected, but not found" ) {
+            context_menu.QAction(:name => 'keepInLauncherAction')['iconText']
+        }
+
+        verify(TIMEOUT, "Quit action was expected, but not found" ) {
+            context_menu.QAction(:name => 'quitAction')
+        }
+
+        XDo::Keyboard.alt_F4
+
+        verify_equal('Remove from launcher', TIMEOUT, "'Remove from launcher' action was expected, but not found" ) {
+            context_menu.QAction(:name => 'keepInLauncherAction')['iconText']
+        }
+
+        verify_equal('false', TIMEOUT, "Remove from launcher action should be unchecked, but it is checked" ) {
+            context_menu.QAction(:name => 'keepInLauncherAction')['checked']
+        }
+
+        verify_not(TIMEOUT, "Quit action should have been removed, but not" ) {
+            context_menu.QAction(:name => 'quitAction')
+        }
+
+        XDo::Mouse.move(context_menu['x_absolute'].to_i + 20, context_menu['y_absolute'].to_i + context_menu['height'].to_i - 10, 0, true)
+        XDo::Mouse.click(nil,nil, :left)
+
+        verify_not( TIMEOUT, 'Could find the test application tile in the launcher, should not be there' ) {
+            tiles = @app.Launcher().LauncherList( :name => 'main' ).children( { :name => 'gedit' } )
+        }
+    end
+  end
+end