Merge lp:~mir-team/qtubuntu/support-state-changing into lp:qtubuntu

Proposed by Robert Carr
Status: Merged
Approved by: Daniel d'Andrada
Approved revision: 247
Merged at revision: 245
Proposed branch: lp:~mir-team/qtubuntu/support-state-changing
Merge into: lp:qtubuntu
Diff against target: 49 lines (+9/-7)
2 files modified
debian/control (+1/-1)
src/ubuntumirclient/window.cpp (+8/-6)
To merge this branch: bzr merge lp:~mir-team/qtubuntu/support-state-changing
Reviewer Review Type Date Requested Status
Daniel d'Andrada (community) Approve
PS Jenkins bot continuous-integration Needs Fixing
Gerry Boland (community) Needs Fixing
Review via email: mp+235546@code.launchpad.net

Commit message

Support mir restored surface state as a way to leave fullscreen.

Description of the change

Support more window states as a solution for: https://bugs.launchpad.net/ubuntu/+source/webbrowser-app/+bug/1328839

Notice comment about weird switch statement...would maybe be good to get some feedback on the original intent of that code.

Depends on:

https://code.launchpad.net/~mir-team/platform-api/support-state-changing/+merge/235545

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :

Reply inline on the weird switch statement

Revision history for this message
Gerry Boland (gerboland) wrote :

Please change debian/control to depend on the new PAPI version

review: Needs Fixing
244. By Robert Carr

Bump papi dependency

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
245. By Robert Carr

Bump papi dep

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
246. By Robert Carr

Merge lp:qtubuntu

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Code looks good, but that TODO comment is confusing. Please remove it.

That "Active|Minimized" in the DLOG message simply means that setWindowState() was called either with Qt::WindowActive or with Qt::WindowMinimized. Nothing more than that. It's just because the author didn't bother writing a separate DLOG message for each one of those two cases.

setWindowState() gets an enumeration, not a bitfield of flags.

review: Needs Fixing
Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Also platform-api has U_MINIMIZED_STATE, why don't you set it there accordingly?

You should ignore Qt::WindowActive. This value is not supported by this method and Qt will never call this method with this value.

Snipped from Qt code:

"""
void QWindow::setWindowState(Qt::WindowState state)
{
    if (state == Qt::WindowActive) {
        qWarning() << "QWindow::setWindowState does not accept Qt::WindowActive";
        return;
    }

    if (d->platformWindow)
        d->platformWindow->setWindowState(state);
"""

Revision history for this message
Robert Carr (robertcarr) wrote :

Ah I see...I was reading the 4.8 documentation where it accepts Qt::WindowStates...updating!

247. By Robert Carr

Remove weird comment, support minimized state

Revision history for this message
Robert Carr (robertcarr) wrote :

Removed comment, setting U_MINIMIZED_STATE. Thanks!

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Carr (robertcarr) wrote :

Jenkins fail because papi isnt available of course.

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Looks good. Thanks.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control'
2--- debian/control 2014-09-19 12:18:13 +0000
3+++ debian/control 2014-09-30 20:46:27 +0000
4@@ -8,7 +8,7 @@
5 libfreetype6-dev,
6 libgles2-mesa-dev,
7 libglib2.0-dev,
8- libubuntu-application-api-dev (>= 2.4.0),
9+ libubuntu-application-api-dev (>= 2.5.0),
10 libudev-dev,
11 libxrender-dev,
12 libxkbcommon-dev,
13
14=== modified file 'src/ubuntumirclient/window.cpp'
15--- src/ubuntumirclient/window.cpp 2014-08-08 14:04:13 +0000
16+++ src/ubuntumirclient/window.cpp 2014-09-30 20:46:27 +0000
17@@ -306,24 +306,26 @@
18 switch (state) {
19 case Qt::WindowNoState:
20 DLOG("setting window state: 'NoState'");
21+ ua_ui_window_request_state(d->window, U_RESTORED_STATE);
22 d->state = Qt::WindowNoState;
23 break;
24-
25 case Qt::WindowFullScreen:
26 DLOG("setting window state: 'FullScreen'");
27- ua_ui_window_request_fullscreen(d->window);
28+ ua_ui_window_request_state(d->window, U_FULLSCREEN_STATE);
29 d->state = Qt::WindowFullScreen;
30 break;
31-
32 case Qt::WindowMaximized:
33 DLOG("setting window state: 'Maximized'");
34+ ua_ui_window_request_state(d->window, U_MAXIMIZED_STATE);
35 d->state = Qt::WindowMaximized;
36 break;
37-
38- case Qt::WindowActive:
39 case Qt::WindowMinimized:
40+ DLOG("setting window state: 'Minimized'");
41+ ua_ui_window_request_state(d->window, U_MINIMIZED_STATE);
42+ d->state = Qt::WindowMinimized;
43+ break;
44 default:
45- DLOG("setting window state: 'Active|Minimized'");
46+ DLOG("Unexpected window state");
47 break;
48 }
49 }

Subscribers

People subscribed via source and target branches