unity-2d

Merge lp://qastaging/~mardy/unity-2d/unmaximize into lp://qastaging/unity-2d

  1. unmaximize
  2. Merge into trunk
Proposed by Alberto Mardegan
Status: Merged
Approved by: Didier Roche-Tolomelli
Approved revision: 759
Merged at revision: 797
Proposed branch: lp://qastaging/~mardy/unity-2d/unmaximize
Merge into: lp://qastaging/unity-2d
Diff against target: 679 lines (+351/-53)
12 files modified
libunity-2d-private/Unity2d/plugin.cpp (+5/-0)
libunity-2d-private/src/CMakeLists.txt (+1/-0)
libunity-2d-private/src/dashsettings.cpp (+117/-0)
libunity-2d-private/src/dashsettings.h (+72/-0)
libunity-2d-private/src/panelstyle.cpp (+6/-0)
libunity-2d-private/src/panelstyle.h (+2/-1)
panel/applets/appname/appnameapplet.cpp (+36/-10)
panel/applets/appname/windowhelper.cpp (+93/-3)
panel/applets/appname/windowhelper.h (+4/-0)
places/app/dashdeclarativeview.cpp (+3/-15)
places/app/places.cpp (+1/-0)
places/dash.qml (+11/-24)
To merge this branch: bzr merge lp://qastaging/~mardy/unity-2d/unmaximize
Reviewer Review Type Date Requested Status
Lohith D Shivamurthy Pending
Michał Sawicz functional Pending
Review via email: mp+83763@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2011-10-05.

Description of the change

(resubmitting without prerequisite branch)

  [dash, panel] Add window controls to dash window

  Show window controls for closing, minimizing and maximizing the Dash.

One potentially big issue:

Although the Dash window state (maximized/unmaximized) is set in DConf, at the next startup the Dash ignores that settings: this is because the Dash itself determines what window mode is best right after it's started. We need to clarify with the UI designers whether this behaviour is the intended one.

To post a comment you must log in.
Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

Works fine here. Only thing I'm not sure of - what's the purpose of minimizing the Dash?

review: Approve (functional)
Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

> Works fine here. Only thing I'm not sure of - what's the purpose of minimizing
> the Dash?
Talking to @fboucault, the minimize button should be disabled / greyed out for the dash.

review: Needs Fixing
Revision history for this message
Alberto Mardegan (mardy) wrote : Posted in a previous version of this proposal

I will change the minimize button not to do anything when pressed, and remove the highlight effect when hovering it.
However, I'm not sure it's the good solution, seeing bug 867910 and bug 862775.

Revision history for this message
Alberto Mardegan (mardy) wrote : Posted in a previous version of this proposal

Merge request updated: the minimize button is now insensitive.

Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

I agree with the bugs mentioned - the minimize button looks out-of-place - broken, not inactive, with that change. Unless we have a "inactive" state of the buttons elsewhere in the desktop - which we don't - we should simply remove the minimize button. Modal windows, for example, only have the close button, not all of them, but with only the close one working.

That will probably require ACK from design.

Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

When we're always fullscreen (Netbook form factor?), the unmaximize button should also be disabled.

Also, when the dash or alt+f2 is active, panel still displays the "Desktop" label.

review: Needs Fixing
Revision history for this message
Alberto Mardegan (mardy) wrote : Posted in a previous version of this proposal

Updated:
- Don't show desktop title when Dash is running
- Disable the maximize/unmaximize button when the screen resolution is too low

Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

Functionally works great, will have to take a longer look at the code, unless someone else wants to do that :]

review: Approve (functional)
Revision history for this message
Lohith D Shivamurthy (dyams) wrote : Posted in a previous version of this proposal

> Functionally works great, will have to take a longer look at the code, unless
> someone else wants to do that :]

I finished reviewing this MR. IMO, We should merge it already.
I have noticed a functional issue though.
But this patch itself is worth more than that small issue. :)

I can log a separate lp bug for that issue later.

Revision history for this message
Lohith D Shivamurthy (dyams) wrote : Posted in a previous version of this proposal

The issue is :
1) Open Dash
2) Make sure its window buttons are not visible.
3) Move the mouse over panel
4) Window buttons(Close, Minimize, Maximize) not visible yet
5) Move the mouse away from panel
6) The Window buttons becomes visible now.

Expected:
at (4) the buttons should have been visible and
at (6) they should have been hidden.

Actual:
Moving the mouse over panel when window buttons are not visible, the buttons are still hidden.
When cursor is moved away, they become visible.

Revision history for this message
Alberto Mardegan (mardy) wrote : Posted in a previous version of this proposal

I'm rather sure that the issue your found was not present when I tested this -- it's a rather big issue that wouldn't pass unnoticed.
However, unfortunately I won't be able to work on this either. :-(

Revision history for this message
Lohith D Shivamurthy (dyams) wrote : Posted in a previous version of this proposal

The code is neat and clean. Happy to approve it.
Thanks a lot.

review: Approve
Revision history for this message
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

Sorry, but make check not passing yet, so merge will fail.

Revision history for this message
Unity Merger (unity-merger) wrote : Posted in a previous version of this proposal

No proposals found for merge of lp:~mardy/unity-2d/spread-geometry into lp:unity-2d.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches