Merge lp:~unity-2d-team/unity-2d/unity-2d-dash-fullscreen-via-dconf into lp:unity-2d

[Ugo Riboni]

[dash][panel] Use a new dash-specific dconf key to store the dash fullscreen state instead of using a wrongly named 3d key

[Florian Boucault (fboucault)]

Code looks great at first. Will carry slightly more detailed code inspection and functional review.

[Florian Boucault (fboucault)]

Let's add the following automated tests to the mix:

17:57 < Kaleo> nerochiaro: 1st test: depending on the dconf key making sure the maximisation state is respected
17:57 < Kaleo> nerochiaro: 2nd test: clicking on the maximise button in the panel; making sure the dash reacts properly
17:58 < nerochiaro> Kaleo: also bring out the dash, change dconf key, check dash has changed state properly

[Florian Boucault (fboucault)]

kaleo@tequila:~/Projects/upicek/unity-2d$ ./places/app/unity-2d-places
unity-2d-places: [WARNING] Object::connect: No such signal DashDeclarativeView::screenGeometryChanged() in /home/kaleo/Projects/upicek/unity-2d/places/app/dashdeclarativeview.cpp:59
unity-2d-places: [WARNING] file:///home/kaleo/Projects/upicek/unity-2d//places/dash.qml:46:16: Unexpected token `return'
             value: return dash2dConfiguration.fullScreen ? DashDeclarativeView.FullScreenMode :

review: Needs Fixing

[Florian Boucault (fboucault)]

The following change does not belong in this merge request.

=== modified file places/app/dashdeclarativeview.cpp
--- places/app/dashdeclarativeview.cpp 2011-12-19 10:54:27 +0000
+++ places/app/dashdeclarativeview.cpp 2012-01-24 16:56:18 +0000
@@ -159,7 +159,6 @@
     if (value != m_active) {
         m_active = value;
         if (value) {
- updateDashModeDependingOnScreenGeometry();
             show();
             raise();
             // We need a delay, otherwise the window may not be visible when we try to activate it

review: Needs Fixing

[Florian Boucault (fboucault)]

Apart from all the comments above, the rest of the code looks great. Manual functional testing showed no new issue.

[Ugo Riboni (uriboni)]

Fixed the error, and pushed the tests.
The commit that you say doesn't belong actually does belong: we need to remove the call to updateDashModeDependingOnScreenGeometry otherwise every time we activate the dash it will override whatever fullscreen value we read from dconf (via the binding in QML).

[Florian Boucault (fboucault)]

> Fixed the error, and pushed the tests.
> The commit that you say doesn't belong actually does belong: we need to remove
> the call to updateDashModeDependingOnScreenGeometry otherwise every time we
> activate the dash it will override whatever fullscreen value we read from
> dconf (via the binding in QML).

Yes but that was already the case in trunk wasn't it? Fixing that is really better done in [1] because in there you change the way monitoring the screen's resolution is done [2] which is completely related to the line of code in question.

[1] https://code.launchpad.net/~unity-2d-team/unity-2d/unity-2d-dash-always-fullscreen-by-resolution/+merge/89886
[2] + connect(QApplication::desktop(), SIGNAL(resized(int)), SLOT(updateAlwaysFullScreen()));

[Florian Boucault (fboucault)]

The tests verify that the dashMode is set properly. It sounds of little use compared to actually checking the size of the dash.

[Florian Boucault (fboucault)]

btw, you will need a test case for what mentioned:
"otherwise every time we activate the dash it will override whatever fullscreen value we read from dconf"

[Ugo Riboni (uriboni)]

I ended up agreeing with pretty much all your comments, therefore the following changes were made:
- Put back the call to updateDashModeDependingOnScreenGeometry on dash activation, so it overrides whatever dconf key is set
- Added a test which verifies this overriding

[Ugo Riboni (uriboni)]

> The tests verify that the dashMode is set properly. It sounds of little use
> compared to actually checking the size of the dash.

The one thing I don't agree with you is this. I think it's more than sufficient to check DashMode, because it's directly tied in QML to dash size and I didn't touch that part at all in this MR.

However if you insist I think that what may be useful is adding just one single test, separate from this MR, which sets a dashMode and tests the size, then sets the other dashMode and tests the size.
It's all it's needed, because the tests in this MR check that dashMode is set to the right value, but trust dashMode to work fine in setting the size. And the other test I propose adding would test that the size is set correctly based on DashMode.

Keep in mind though that the tests for shaping that are in the shell branch will do exactly these kinds of tests, so it all feels quite redundant.

[Florian Boucault (fboucault)]

> > The tests verify that the dashMode is set properly. It sounds of little use
> > compared to actually checking the size of the dash.
>
> The one thing I don't agree with you is this. I think it's more than
> sufficient to check DashMode, because it's directly tied in QML to dash size
> and I didn't touch that part at all in this MR.
>
> However if you insist I think that what may be useful is adding just one
> single test, separate from this MR, which sets a dashMode and tests the size,
> then sets the other dashMode and tests the size.
> It's all it's needed, because the tests in this MR check that dashMode is set
> to the right value, but trust dashMode to work fine in setting the size. And
> the other test I propose adding would test that the size is set correctly
> based on DashMode.
>
> Keep in mind though that the tests for shaping that are in the shell branch
> will do exactly these kinds of tests, so it all feels quite redundant.

If we have tests down the line that will do it, then we are good.

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity-2d/86/console reported an error when processing this lp:~unity-2d-team/unity-2d/unity-2d-dash-fullscreen-via-dconf branch.
Not merging it.

=== modified file 'config.h.in'
--- config.h.in	2011-12-07 19:20:54 +0000
+++ config.h.in	2012-01-26 18:08:30 +0000
@@ -11,6 +11,7 @@
 static const char* UNITY2D_DCONF_SCHEMA = "com.canonical.Unity2d";
 static const char* LAUNCHER2D_DCONF_SCHEMA = "com.canonical.Unity2d.Launcher";
 static const char* PANEL2D_DCONF_SCHEMA = "com.canonical.Unity2d.Panel";
+static const char* DASH2D_DCONF_SCHEMA = "com.canonical.Unity2d.Dash";
 
 static const char* UNITY_DCONF_SCHEMA = "com.canonical.Unity";
 static const char* LAUNCHER_DCONF_SCHEMA = "com.canonical.Unity.Launcher";
@@ -31,6 +32,11 @@
     return configuration;
 }
 
+inline QConf& dash2dConfiguration() {
+    static QConf configuration(DASH2D_DCONF_SCHEMA);
+    return configuration;
+}
+
 inline QConf& unityConfiguration() {
     static QConf configuration(UNITY_DCONF_SCHEMA);
     return configuration;

=== modified file 'data/com.canonical.Unity2d.gschema.xml'
--- data/com.canonical.Unity2d.gschema.xml	2011-12-01 09:55:45 +0000
+++ data/com.canonical.Unity2d.gschema.xml	2012-01-26 18:08:30 +0000
@@ -36,4 +36,14 @@
       </description>
     </key>
   </schema>
+  <schema path="/com/canonical/unity-2d/dash/" id="com.canonical.Unity2d.Dash" gettext-domain="unity-2d">
+    <key type="b" name="full-screen">
+      <default>false</default>
+      <summary>The current fullscreen status of the dash</summary>
+      <description>
+        Describe if the dash should be fullscreen or not. If the dash is already running this setting
+        is applied immediately, otherwise it is applied as soon as the dash is activated again.
+      </description>
+    </key>
+  </schema>
 </schemalist>

=== modified file 'libunity-2d-private/Unity2d/plugin.cpp'
--- libunity-2d-private/Unity2d/plugin.cpp	2012-01-19 14:35:41 +0000
+++ libunity-2d-private/Unity2d/plugin.cpp	2012-01-26 18:08:30 +0000
@@ -179,6 +179,9 @@
     engine->rootContext()->setContextProperty("screen", ScreenInfo::instance());
     engine->rootContext()->setContextProperty("iconUtilities", new IconUtilities(engine));
 
+    /* Expose QConf objects as a context property not to initialize it multiple times */
+    engine->rootContext()->setContextProperty("dash2dConfiguration", &dash2dConfiguration());
+
     /* Critically important to set the client type to pager because wnck
        will pass that type over to the window manager through XEvents.
        Window managers tend to respect orders from pagers to the letter by

=== modified file 'libunity-2d-private/src/dashsettings.cpp'
--- libunity-2d-private/src/dashsettings.cpp	2011-12-09 09:14:11 +0000
+++ libunity-2d-private/src/dashsettings.cpp	2012-01-26 18:08:30 +0000
@@ -21,73 +21,16 @@
 // Self
 #include "dashsettings.h"
 
-// Local
-
-// Qt
-#include <QMetaEnum>
-#include <QSize>
-#include <QVariant>
-
 #include <config.h>
 
 static const int DASH_MIN_SCREEN_WIDTH = 1280;
 static const int DASH_MIN_SCREEN_HEIGHT = 1084;
 
-static const char* FORM_FACTOR = "formFactor";
-
 namespace Unity2d {
 
-class DashSettingsPrivate
-{
-    DashSettingsPrivate(DashSettings *dashSettings)
-    {
-        /* proxy the formFactor property signal notifier */
-        QObject::connect(&unityConfiguration(), SIGNAL(formFactorChanged(QString)),
-                         dashSettings, SIGNAL(formFactorChanged()));
-
-        /* Get the QMetaEnum for FormFactor: we will use it when setting and
-         * getting the property, to convert the string value returned by QConf
-         * to an enum value, and vice versa. */
-        const QMetaObject *metaObject = dashSettings->metaObject();
-        int index = metaObject->indexOfEnumerator("FormFactor");
-        formFactorEnum = metaObject->enumerator(index);
-    }
-
-    ~DashSettingsPrivate()
-    {
-    }
-
-private:
-    DashSettings* q_ptr;
-    Q_DECLARE_PUBLIC(DashSettings)
-
-    QMetaEnum formFactorEnum;
-};
-
 DashSettings::DashSettings(QObject* parent):
-    QObject(parent),
-    d_ptr(new DashSettingsPrivate(this))
-{
-}
-
-DashSettings::~DashSettings()
-{
-    delete d_ptr;
-}
-
-void DashSettings::setFormFactor(FormFactor formFactor)
-{
-    Q_D(DashSettings);
-
-    const char* name = d->formFactorEnum.valueToKey(formFactor);
-    unityConfiguration().setProperty(FORM_FACTOR, QVariant(QString::fromLatin1(name)));
-}
-
-DashSettings::FormFactor DashSettings::formFactor() const
-{
-    Q_D(const DashSettings);
-    QByteArray key = unityConfiguration().property(FORM_FACTOR).toString().toLatin1();
-    return FormFactor(d->formFactorEnum.keyToValue(key.constData()));
+    QObject(parent)
+{
 }
 
 static int getenvInt(const char* name, int defaultValue)

=== modified file 'libunity-2d-private/src/dashsettings.h'
--- libunity-2d-private/src/dashsettings.h	2011-10-17 12:12:10 +0000
+++ libunity-2d-private/src/dashsettings.h	2012-01-26 18:08:30 +0000
@@ -23,12 +23,10 @@
 
 // Qt
 #include <QObject>
-#include <QMetaType>
+#include <QSize>
 
 namespace Unity2d {
 
-class DashSettingsPrivate;
-
 /**
  * Provide easy access to the Dash settings (from Qt and QML)
  */
@@ -36,37 +34,15 @@
 {
     Q_OBJECT
 
-    Q_ENUMS(FormFactor)
-
-    Q_PROPERTY(FormFactor formFactor READ formFactor WRITE setFormFactor NOTIFY formFactorChanged)
-
 public:
     explicit DashSettings(QObject* parent = 0);
-    ~DashSettings();
-
-    enum FormFactor
-    {
-        Automatic = 0,
-        Desktop,
-        Netbook
-    };
-
-    void setFormFactor(FormFactor formFactor);
-    FormFactor formFactor() const;
 
     static QSize minimumSizeForDesktop();
 
-Q_SIGNALS:
-    void formFactorChanged();
-
 private:
     Q_DISABLE_COPY(DashSettings)
-    DashSettingsPrivate* d_ptr;
-    Q_DECLARE_PRIVATE(DashSettings)
 };
 
 } // namespace Unity2d
 
-Q_DECLARE_METATYPE(Unity2d::DashSettings::FormFactor)
-
 #endif /* DASHSETTINGS_H */

=== modified file 'panel/applets/appname/windowhelper.cpp'
--- panel/applets/appname/windowhelper.cpp	2011-12-02 04:22:53 +0000
+++ panel/applets/appname/windowhelper.cpp	2012-01-26 18:08:30 +0000
@@ -23,6 +23,7 @@
 #include "windowhelper.h"
 
 // Local
+#include "config.h"
 
 // unity-2d
 #include <dashsettings.h>
@@ -45,6 +46,7 @@
 #include <QApplication>
 #include <QDesktopWidget>
 #include <QPoint>
+#include <QVariant>
 
 // X11
 #include <X11/Xlib.h>
@@ -123,12 +125,6 @@
     QMetaObject::invokeMethod(watcher, "nameChanged");
 }
 
-static void geometryChangedCB(GObject* window,
-    WindowHelper*  watcher)
-{
-    QMetaObject::invokeMethod(watcher, "stateChanged");
-}
-
 void WindowHelper::update()
 {
     BamfWindow* bamfWindow = BamfMatcher::get_default().active_window();
@@ -144,13 +140,6 @@
         const char *name = wnck_window_get_name(d->m_window);
         d->m_activeWindowIsDash = qstrcmp(name, "unity-2d-places") == 0;
         if (d->m_activeWindowIsDash) {
-            /* Since we are not really minimizing and maximizing the dash we
-             * cannot rely on the wnck "state-changed" signal to be emitted;
-             * instead, listen for the "geometry-changed" and emit our
-             * stateChanged() from that. */
-            d->m_connector.connect(G_OBJECT(d->m_window), "geometry-changed",
-                                   G_CALLBACK(geometryChangedCB), this);
-
             d->updateDashCanResize();
         }
 
@@ -167,11 +156,7 @@
         return false;
     }
     if (d->m_activeWindowIsDash) {
-        int x, y, width, height;
-        wnck_window_get_geometry(d->m_window, &x, &y, &width, &height);
-        const QRect windowGeometry(x, y, width, height);
-        ScreenInfo* screenInfo = ScreenInfo::instance();
-        return screenInfo->panelsFreeGeometry() == windowGeometry;
+        return dash2dConfiguration().property("fullScreen").toBool();
     } else {
         return wnck_window_is_maximized(d->m_window);
     }
@@ -226,7 +211,7 @@
 void WindowHelper::maximize()
 {
     if (d->m_activeWindowIsDash) {
-        d->m_dashSettings->setFormFactor(DashSettings::Netbook);
+        dash2dConfiguration().setProperty("fullScreen", QVariant(true));
     } else {
         /* This currently cannot happen, because the window buttons are not
          * shown in the panel for non maximized windows. It's here just for
@@ -238,7 +223,7 @@
 void WindowHelper::unmaximize()
 {
     if (d->m_activeWindowIsDash) {
-        d->m_dashSettings->setFormFactor(DashSettings::Desktop);
+        dash2dConfiguration().setProperty("fullScreen", QVariant(false));
     } else {
         wnck_window_unmaximize(d->m_window);
     }

=== modified file 'places/dash.qml'
--- places/dash.qml	2012-01-03 11:55:41 +0000
+++ places/dash.qml	2012-01-26 18:08:30 +0000
@@ -40,15 +40,11 @@
         value: (currentPage && currentPage.expanded != undefined) ? currentPage.expanded : true
     }
 
-    DashSettings {
-        id: settings
-    }
-
     Binding {
         target: dashView
         property: "dashMode"
-        value: (settings.formFactor == DashSettings.Desktop) ?
-            DashDeclarativeView.DesktopMode : DashDeclarativeView.FullScreenMode
+        value: dash2dConfiguration.fullScreen ? DashDeclarativeView.FullScreenMode :
+                                                DashDeclarativeView.DesktopMode
     }
 
     function activatePage(page) {

=== added file 'tests/places/fullscreen.rb'
--- tests/places/fullscreen.rb	1970-01-01 00:00:00 +0000
+++ tests/places/fullscreen.rb	2012-01-26 18:08:30 +0000
@@ -0,0 +1,168 @@
+#!/usr/bin/env ruby1.8
+=begin
+/*
+ * This file is part of unity-2d
+ *
+ * Copyright 2011 Canonical Ltd.
+ *
+ * Authors:
+ * - Ugo Riboni <[e-mail]>
+ * - Gerry Boland <[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'
+require 'timeout'
+require 'tmpdir'
+require 'tempfile'
+include TDriverVerify
+
+############################# Test Suite #############################
+context "Dash fullscreen tests" do
+  pwd = File.expand_path(File.dirname(__FILE__)) + '/'
+
+  DASH_FULLSCREEN_KEY = '/com/canonical/unity-2d/dash/full-screen'
+  DASH_MIN_SCREEN_WIDTH = 1280;
+  DASH_MIN_SCREEN_HEIGHT = 1084;
+
+  # Run once at the beginning of this test suite
+  startup do
+    system 'killall unity-2d-places > /dev/null 2>&1'
+    system 'killall unity-2d-panel > /dev/null 2>&1'
+    system 'killall unity-2d-launcher > /dev/null 2>&1'
+    system 'killall unity-2d-launcher > /dev/null 2>&1'
+
+    # Minimize all windows
+    XDo::XWindow.toggle_minimize_all
+  end
+  
+  # Run once at the end of this test suite
+  shutdown do
+  end
+
+  # Run before each test case begins
+  setup do
+    @oldvalue = %x{dconf read #{DASH_FULLSCREEN_KEY}}.chop
+
+    # Execute the application
+    @sut = TDriver.sut(:Id => "sut_qt")
+    @dash = @sut.run(:name => UNITY_2D_PLACES,
+                    :arguments => "-testability",
+                    :sleeptime => 2)
+    @panel = @sut.run(:name => UNITY_2D_PANEL,
+                    :arguments => "-testability",
+                    :sleeptime => 2)
+    @launcher = @sut.run(:name => UNITY_2D_LAUNCHER,
+                         :arguments => "-testability",
+                         :sleeptime => 2)
+
+    verify(10){ @panel.Unity2dPanel() }
+    verify(10){ @launcher.Unity2dPanel() }
+    # Dash can't be verified here as the declarative view is hidden until we make it active
+  end
+
+  # Run after each test case completes
+  teardown do
+    %x{dconf write #{DASH_FULLSCREEN_KEY} #{@oldvalue}}
+    system "pkill -nf unity-2d-panel"
+    system "pkill -nf unity-2d-places"
+    system "pkill -nf unity-2d-launcher"
+  end
+
+  def dash_always_fullscreen
+    out = %x{xdotool getdisplaygeometry}
+    width, height = out.split.collect { |coord| coord.to_i }
+    return width < DASH_MIN_SCREEN_WIDTH && height < DASH_MIN_SCREEN_HEIGHT
+  end
+
+  #####################################################################################
+  # Test cases
+
+  test "Dash fullscreens on dconf key change" do
+    %x{dconf write #{DASH_FULLSCREEN_KEY} false}
+    XDo::Keyboard.super
+    sleep 1
+
+    verify_equal('true', TIMEOUT, 'Dash did not appear') {
+        @dash.DashDeclarativeView()['active']
+    }
+    verify_equal('DesktopMode', TIMEOUT, 'Dash is fullscreen but should not be') {
+        @dash.DashDeclarativeView()['dashMode']
+    }
+
+    %x{dconf write #{DASH_FULLSCREEN_KEY} true}
+    sleep 1
+
+    verify_equal('FullScreenMode', TIMEOUT, 'Dash is not fullscreen but should be') {
+        @dash.DashDeclarativeView()['dashMode']
+    }
+  end
+
+  test "Dash reacts correctly to panel buttons" do
+    oldvalue = %x{dconf read #{DASH_FULLSCREEN_KEY}}
+
+    %x{dconf write #{DASH_FULLSCREEN_KEY} false}
+    XDo::Keyboard.super
+    sleep 1
+    verify_equal('true', TIMEOUT, 'Dash did not appear') {
+        @dash.DashDeclarativeView()['active']
+    }
+
+    maxbutton = nil
+    verify(TIMEOUT, 'The "maximize" button did not appear when the dash was visible' ) {
+        maxbutton = @panel.AppNameApplet().children( :type => 'QAbstractButton' )[2]
+    }
+
+    maxbutton.tap if maxbutton
+    sleep 1
+    verify_equal('FullScreenMode', TIMEOUT, 'Dash should be fullsceen, but it is not' ) {
+        @dash.DashDeclarativeView()['dashMode']
+    }
+    verify_equal('true', TIMEOUT, 'Dash fullscreen key was not set') {
+        %x{dconf read #{DASH_FULLSCREEN_KEY}}.chop
+    }
+
+    maxbutton.tap if maxbutton
+    sleep 1
+    verify_equal('DesktopMode', TIMEOUT, 'Dash should not be fullsceen, but it is' ) {
+        @dash.DashDeclarativeView()['dashMode']
+    }
+    verify_equal('false', TIMEOUT, 'Dash fullscreen key was not unset') {
+        %x{dconf read #{DASH_FULLSCREEN_KEY}}.chop
+    }
+  end
+
+  test "Dash fullscreen initially" do
+    # The initial startup mode ignores the dconf key and only decides based on the screen
+    # resolution. It's a bug and will be fixed, but as long as it's there let's test for it.
+
+    expect = dash_always_fullscreen ? 'FullScreenMode' : 'DesktopMode'
+    initial = dash_always_fullscreen ? 'false' : 'true'
+    %x{dconf write #{DASH_FULLSCREEN_KEY} #{initial}}
+    XDo::Keyboard.super
+    sleep 1
+
+    verify_equal('true', TIMEOUT, 'Dash did not appear') {
+        @dash.DashDeclarativeView()['active']
+    }
+
+    verify_equal(expect, TIMEOUT, 'Dash initial state is wrong') {
+        @dash.DashDeclarativeView()['dashMode']
+    }
+  end
+end