Merge lp://qastaging/~agateau/unity-2d/desktop-dash into lp://qastaging/unity-2d/3.0

- clicking on the Ubuntu circle-of-friends in the panel does not show a miminized version of the dash.
- clicking on a place in the launcher does not show a windowed version of the dash

Please add a commit message that will be used automatically when the branch lands.

In places/app/dashdeclarativeview.h:

    enum DashState {
        HiddenDash,
        CollapsedDesktopDash, /// Only search line-edit and "Shortcuts" button
        ExpandedDesktopDash, /// CollapsedDesktopDash + search results
        FullScreenDash
    };

I don't think the values need to contain 'Dash' in their names. Proposal:
- Hidden
-Collapsed
- Expanded
- Fullscreen

In places/app/dashdeclarativeview.h:

Property dashState should not contain 'dash' in its name (similar to the 'active' property which is not named 'dashActive'). All methods (getter, setter, signal) should be renamed accordingly.

DashDeclarativeView::onWorkAreaResized(int screen)

should take into account other states than just 'FullScreenDash'.

DashDeclarativeView::setDashState(DashDeclarativeView::DashState state)

should call resize (calling fitToAvailableSpace or resizeToDesktopDashSize) _before_ calling 'show'.

> DashDeclarativeView::onWorkAreaResized(int screen)
>
> should take into account other states than just 'FullScreenDash'.

If the available work area's size becomes "close enough" to the size of dash in 'desktop' state then the dash should take all the width of the screen and drop its window borders and resizing handle (similar to what happens in 'fullscreen' state).

To ease that implementation and make the code cleaner, the logic to determine if the size of the dash is "close enough" should not be part of DashDeclarativeView::setActive but factored out in a separate method.
In this merge request that code is incomplete at the moment as it only checks the resolution of the screen against a static resolution DASH_MIN_SCREEN_WIDTH/DASH_MIN_SCREEN_HEIGHT whereas it should:
 - determine whether or not the screen resolution is one of a netbook
 - if yes, then compare the size of the dash in 'desktop' state and the size of the available area and if the dash takes "most" of the available area then behave as if it were in 'fullscreen' state

Let's discuss this very soon in order to make it crystal clear.

places/artwork/desktop_dash.sci
places/artwork/desktop_dash_background.png

Please name the sci identically to the png file. Also I think it would be clearer to name it 'border' or 'handle' rather than 'background'

In places/app/places.cpp:

- view.setAttribute(Qt::WA_OpaquePaintEvent);
- view.setAttribute(Qt::WA_NoSystemBackground);
+ view.setAttribute(Qt::WA_TranslucentBackground);
+ view.viewport()->setAttribute(Qt::WA_TranslucentBackground);

Until Metacity has compositing activated by default that code should be conditional. Please restore the previous code by default.

places/Home.qml

- opacity: globalSearchActive ? 0 : 1
+ opacity: globalSearchActive || dashView.dashState == DashDeclarativeView.CollapsedDesktopDash ? 0 : 1

This change does not look necessary.

places/dash.qml:

- 'shortcutsButton' should be part of the home page as it is never used in the places.
- 'expanded'/'collapsed' state management is overly complicated: a binding of dashView.dashState would suffice:

Binding {
    target: dashView
    property: "dashState"
    value: THE_RIGHT_VALUE (taken from updateDashState)
}

Though this will not be necessary as this should move to Home.qml as well as it is the only place where collapsed/expanded is relevant

places/PlaceEntryView.qml

New 'expanded' property should be removed (as a consequence of previous comment).

> - clicking on the Ubuntu circle-of-friends in the panel does not show a
> miminized version of the dash.
> - clicking on a place in the launcher does not show a windowed version of the
> dash

It works here. What is the size of your screen? maybe you fall in the "fullscreen" only case?

> In places/app/dashdeclarativeview.h:
>
> enum DashState {
> HiddenDash,
> CollapsedDesktopDash, /// Only search line-edit and "Shortcuts" button
> ExpandedDesktopDash, /// CollapsedDesktopDash + search results
> FullScreenDash
> };
>
>
> I don't think the values need to contain 'Dash' in their names. Proposal:
> - Hidden
> -Collapsed
> - Expanded
> - Fullscreen

I disagree. See http://doc.qt.nokia.com/qq/qq13-apis.html#theartofnaming for the rational behind these names.

> In places/app/dashdeclarativeview.h:
>
> Property dashState should not contain 'dash' in its name (similar to the
> 'active' property which is not named 'dashActive'). All methods (getter,
> setter, signal) should be renamed accordingly.

I disagree: "state" is too generic. I agree "dash" is not the best prefix though (same for the enums). What about renaming the whole "dashState" to "appearance" or "formFactor"?

> In this merge request that code is incomplete at the moment as it only checks
> the resolution of the screen against a static resolution
> DASH_MIN_SCREEN_WIDTH/DASH_MIN_SCREEN_HEIGHT whereas it should:

The spec explicitly defines these resolution limits.

> DashDeclarativeView::setDashState(DashDeclarativeView::DashState state)
>
> should call resize (calling fitToAvailableSpace or resizeToDesktopDashSize)
> _before_ calling 'show'.

It does not really matter because X is asynchronous: the window won't appear on the screen until we are back to the event loop.

> places/artwork/desktop_dash.sci
> places/artwork/desktop_dash_background.png
>
> Please name the sci identically to the png file.

Oups. I agree.

> Also I think it would be
> clearer to name it 'border' or 'handle' rather than 'background'

I choose background because the image is also used for the background of the Dash. I don't have any strong opinion on this though: pick which name you prefer.

> In places/app/places.cpp:
>
> - view.setAttribute(Qt::WA_OpaquePaintEvent);
> - view.setAttribute(Qt::WA_NoSystemBackground);
> + view.setAttribute(Qt::WA_TranslucentBackground);
> + view.viewport()->setAttribute(Qt::WA_TranslucentBackground);
>
> Until Metacity has compositing activated by default that code should be
> conditional. Please restore the previous code by default.

It is going to be super-ugly if I do this. Should I detect compositing and define a window mask if it's not available?

> places/Home.qml
>
> - opacity: globalSearchActive ? 0 : 1
> + opacity: globalSearchActive || dashView.dashState ==
> DashDeclarativeView.CollapsedDesktopDash ? 0 : 1
>
> This change does not look necessary.

It is necessary, otherwise the home buttons appears behind the search widget in Collapsed mode.

> > In places/app/dashdeclarativeview.h:
> >
> > enum DashState {
> > HiddenDash,
> > CollapsedDesktopDash, /// Only search line-edit and "Shortcuts"
> button
> > ExpandedDesktopDash, /// CollapsedDesktopDash + search results
> > FullScreenDash
> > };
> >
> >
> > I don't think the values need to contain 'Dash' in their names. Proposal:
> > - Hidden
> > -Collapsed
> > - Expanded
> > - Fullscreen
>
> I disagree. See http://doc.qt.nokia.com/qq/qq13-apis.html#theartofnaming for
> the rational behind these names.

I don't see how it applies here. Note that we are not polluting the Qt namespace here. Please detail your point.

> > - clicking on the Ubuntu circle-of-friends in the panel does not show a
> > miminized version of the dash.
> > - clicking on a place in the launcher does not show a windowed version of
> the
> > dash
>
> It works here. What is the size of your screen? maybe you fall in the
> "fullscreen" only case?

1280x800 is my screen size.

> > > In places/app/dashdeclarativeview.h:
> > >
> > > enum DashState {
> > > HiddenDash,
> > > CollapsedDesktopDash, /// Only search line-edit and "Shortcuts"
> > button
> > > ExpandedDesktopDash, /// CollapsedDesktopDash + search results
> > > FullScreenDash
> > > };
> > >
> > >
> > > I don't think the values need to contain 'Dash' in their names. Proposal:
> > > - Hidden
> > > -Collapsed
> > > - Expanded
> > > - Fullscreen
> >
> > I disagree. See http://doc.qt.nokia.com/qq/qq13-apis.html#theartofnaming for
> > the rational behind these names.
>
>
> I don't see how it applies here. Note that we are not polluting the Qt
> namespace here. Please detail your point.

The link I posted is not about Qt itself, it is about Qt-style API, which is what we are creating. An enum named Fullscreen or Expanded is ambiguous, hence the need for a suffix, has suggested in the "Naming Enum Types and Values" chapter of the link.

> > In places/app/dashdeclarativeview.h:
> >
> > Property dashState should not contain 'dash' in its name (similar to the
> > 'active' property which is not named 'dashActive'). All methods (getter,
> > setter, signal) should be renamed accordingly.
>
> I disagree: "state" is too generic. I agree "dash" is not the best prefix
> though (same for the enums). What about renaming the whole "dashState" to
> "appearance" or "formFactor"?

How about 'mode' or 'layout'? or 'layoutMode'?

> > places/artwork/desktop_dash.sci
> > places/artwork/desktop_dash_background.png
> >
> > Please name the sci identically to the png file.
>
> Oups. I agree.
>
> > Also I think it would be
> > clearer to name it 'border' or 'handle' rather than 'background'
>
> I choose background because the image is also used for the background of the
> Dash. I don't have any strong opinion on this though: pick which name you
> prefer.

I had not noticed it was used as background. That probably conflicts with the GnomeBackground being drawn below with a color blended into it. Let's discuss this.

> > > - clicking on the Ubuntu circle-of-friends in the panel does not show a
> > > miminized version of the dash.
> > > - clicking on a place in the launcher does not show a windowed version of
> > the
> > > dash
> >
> > It works here. What is the size of your screen? maybe you fall in the
> > "fullscreen" only case?
>
> 1280x800 is my screen size.

That's it: the spec says if both your screen dimensions are less than or equal to 1280 x 1024, only the fullscreen dash should be displayed.

You can work-around it through two environment variables: Start unity-2d-places like this:

DASH_MIN_SCREEN_WIDTH=640 DASH_MIN_SCREEN_HEIGHT=480 unity-2d-places

(Actually only setting the HEIGHT should be enough)

> > places/Home.qml
> >
> > - opacity: globalSearchActive ? 0 : 1
> > + opacity: globalSearchActive || dashView.dashState ==
> > DashDeclarativeView.CollapsedDesktopDash ? 0 : 1
> >
> > This change does not look necessary.
>
> It is necessary, otherwise the home buttons appears behind the search widget
> in Collapsed mode.

My bad.

> > > places/artwork/desktop_dash.sci
> > > places/artwork/desktop_dash_background.png
> > >
> > > Please name the sci identically to the png file.
> >
> > Oups. I agree.
> >
> > > Also I think it would be
> > > clearer to name it 'border' or 'handle' rather than 'background'
> >
> > I choose background because the image is also used for the background of the
> > Dash. I don't have any strong opinion on this though: pick which name you
> > prefer.
>
> I had not noticed it was used as background. That probably conflicts with the
> GnomeBackground being drawn below with a color blended into it. Let's discuss
> this.

Right now what happens is that GnomeBackground is not shown when in desktop mode, so it does not really conflict. That may not be what the designer had in mind though.

> places/dash.qml:
>
> - 'shortcutsButton' should be part of the home page as it is never used in the
> places.

This cannot be done because the home page is hidden when the shortcut button is shown.

> - 'expanded'/'collapsed' state management is overly complicated: a binding of
> dashView.dashState would suffice:
>
> Binding {
> target: dashView
> property: "dashState"
> value: THE_RIGHT_VALUE (taken from updateDashState)
> }
>
> Though this will not be necessary as this should move to Home.qml as well as
> it is the only place where collapsed/expanded is relevant

Just tried this approach and could not get it to work: it caused a loop in the binding as the computation for THE_RIGHT_VALUE needs to access dashState (to avoid changing it when we are in fullscreen mode)

>> places/artwork/desktop_dash.sci
>> places/artwork/desktop_dash_background.png
>>
>> Please name the sci identically to the png file.

> Oups. I agree.

Renamed desktop_dash.sci to desktop_dash_background.sci for now.

> > places/dash.qml:
> >
> > - 'shortcutsButton' should be part of the home page as it is never used in
> the
> > places.
>
> This cannot be done because the home page is hidden when the shortcut button
> is shown.

OK, I found a way to do this.
>
> > - 'expanded'/'collapsed' state management is overly complicated: a binding
> of
> > dashView.dashState would suffice:
> >
> > Binding {
> > target: dashView
> > property: "dashState"
> > value: THE_RIGHT_VALUE (taken from updateDashState)
> > }
> >
> > Though this will not be necessary as this should move to Home.qml as well as
> > it is the only place where collapsed/expanded is relevant
>
> Just tried this approach and could not get it to work: it caused a loop in the
> binding as the computation for THE_RIGHT_VALUE needs to access dashState (to
> avoid changing it when we are in fullscreen mode)

Just committed a new version, which is quite simpler and does not require change in PlaceViewEntry.qml. This was possible because I got rid of the two different Desktop states and added a separate "expanded" boolean property. Code is simpler now.

I also renamed DashState to DashMode and its values: HiddenMode, DesktopMode and FullScreenMode.

Added a semi decent fall-back for non composite mode. I think I addressed all your issues now.

I'm still going through the log of comments/replies, please bear with me.

One thing I noticed during functional testing is that in expanded desktop mode the 'fullscreen' button does not work anymore.

Looks like:

    Binding {
        target: dashView
        property: "expanded"
        value: (currentPage && currentPage.expanded != undefined) ? currentPage.expanded : true
    }

could be moved from places/dash.qml to places/Home.qml

Other than that all the other changes look great especially the simplifications to the QML which are so neat! That will make merging the work with the new look of the dash trivial.

We still have the screen size thing to discuss now that I found the lost specification again :)

After discussion it was decided the following:

In non composite mode, a different PNG image needs to be used for the border and background. It will be fully opaque and serves also as the mask. The background color (part behind the content/search results) is to be nearly black: value of 0.15.
Also in non composite mode the flags that were used before should be readded:
    /* Performance tricks */
    view.setAttribute(Qt::WA_OpaquePaintEvent);
    view.setAttribute(Qt::WA_NoSystemBackground);

Deciding whether or not to make the dash fullscreen by default should use a strict comparison instead of a greater-or-equal.

Content/search results should not overlap the border.

When the dash was shown, it used to receive all the key presses thus allowing the user to type a search query. With this MR it seems broken.

> Looks like:
>
> Binding {
> target: dashView
> property: "expanded"
> value: (currentPage && currentPage.expanded != undefined) ?
> currentPage.expanded : true
> }
>
>
> could be moved from places/dash.qml to places/Home.qml

The binding depends on currentPage, which is in dash.qml, so what would be the gain of moving it?

> When the dash was shown, it used to receive all the key presses thus allowing the user to type a search query. With this MR it seems broken.

Yes I noticed that. Will fix.

> > Looks like:
> >
> > Binding {
> > target: dashView
> > property: "expanded"
> > value: (currentPage && currentPage.expanded != undefined) ?
> > currentPage.expanded : true
> > }
> >
> >
> > could be moved from places/dash.qml to places/Home.qml
>
> The binding depends on currentPage, which is in dash.qml, so what would be the
> gain of moving it?

Moving it to places/Home.qml also means using a reference to the home page instead of 'currentPage'. The point here is that the behaviour is specific to the home page.

>
> > When the dash was shown, it used to receive all the key presses thus
> allowing the user to type a search query. With this MR it seems broken.
>
> Yes I noticed that. Will fix.

According to the mockups, in composite mode, the GnomeBackground should never be used, not even in fullscreen mode. dash.qml needs changing:

     GnomeBackground {
         [...]
         visible: dashView.dashMode == DashDeclarativeView.FullScreenMode
     }

> According to the mockups, in composite mode, the GnomeBackground should never
> be used, not even in fullscreen mode. dash.qml needs changing:
>
> GnomeBackground {
> [...]
> visible: dashView.dashMode == DashDeclarativeView.FullScreenMode
> }

    BorderImage {
        [...]
        visible: dashView.dashMode == DashDeclarativeView.DesktopMode

also needs changing to be always visible when in composite mode.

> > According to the mockups, in composite mode, the GnomeBackground should
> never
> > be used, not even in fullscreen mode. dash.qml needs changing:
> >
> > GnomeBackground {
> > [...]
> > visible: dashView.dashMode == DashDeclarativeView.FullScreenMode
> > }
>
> BorderImage {
> [...]
> visible: dashView.dashMode == DashDeclarativeView.DesktopMode
>
>
> also needs changing to be always visible when in composite mode.

Actually, that last comment is incorrect. What we need is a black background with 69% opacity.

> > > Looks like:
> > >
> > > Binding {
> > > target: dashView
> > > property: "expanded"
> > > value: (currentPage && currentPage.expanded != undefined) ?
> > > currentPage.expanded : true
> > > }
> > >
> > >
> > > could be moved from places/dash.qml to places/Home.qml
> >
> > The binding depends on currentPage, which is in dash.qml, so what would be
> the
> > gain of moving it?
>
> Moving it to places/Home.qml also means using a reference to the home page
> instead of 'currentPage'. The point here is that the behaviour is specific to
> the home page.

We need 'currentPage' to be able to turn expanded on when switching to the place view.

The dash now appears in the list of running applications both in the launcher and in alt+tab.

I believe this is due to revision 411.

> > > > Looks like:
> > > >
> > > > Binding {
> > > > target: dashView
> > > > property: "expanded"
> > > > value: (currentPage && currentPage.expanded != undefined) ?
> > > > currentPage.expanded : true
> > > > }
> > > >
> > > >
> > > > could be moved from places/dash.qml to places/Home.qml
> > >
> > > The binding depends on currentPage, which is in dash.qml, so what would be
> > the
> > > gain of moving it?
> >
> > Moving it to places/Home.qml also means using a reference to the home page
> > instead of 'currentPage'. The point here is that the behaviour is specific
> to
> > the home page.
>
> We need 'currentPage' to be able to turn expanded on when switching to the
> place view.

You are right. Sorry for overlooking this.

About "Content/search results should not overlap the border.", I believe the correct values are:

        anchors.rightMargin: 37
        anchors.bottomMargin: 39

That is for the item containing all the content (= item containing search_entry, pageLoader, etc.)

Also I guess the fullScreenButton will have to be moved out of it.

Attempt to merge into lp:unity-2d failed due to conflicts:

text conflict in places/app/CMakeLists.txt
text conflict in places/app/places.cpp