qtubuntu

Merge lp://qastaging/~unity-team/qtubuntu/DPR into lp://qastaging/qtubuntu

  1. DPR
  2. Merge into trunk
Proposed by Gerry Boland
Status: Superseded
Proposed branch: lp://qastaging/~unity-team/qtubuntu/DPR
Merge into: lp://qastaging/qtubuntu
Prerequisite: lp://qastaging/~dandrader/qtubuntu/loggingCategory
Diff against target: 767 lines (+229/-152)
9 files modified
debian/changelog (+6/-0)
src/ubuntumirclient/backingstore.cpp (+11/-5)
src/ubuntumirclient/input.cpp (+15/-12)
src/ubuntumirclient/screen.cpp (+24/-15)
src/ubuntumirclient/screen.h (+2/-0)
src/ubuntumirclient/ubuntumirclient.pro (+2/-1)
src/ubuntumirclient/utils.h (+29/-0)
src/ubuntumirclient/window.cpp (+136/-117)
src/ubuntumirclient/window.h (+4/-2)
To merge this branch: bzr merge lp://qastaging/~unity-team/qtubuntu/DPR
Reviewer Review Type Date Requested Status
Daniel d'Andrada (community) Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+280820@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2015-04-27.

This proposal has been superseded by a proposal from 2016-01-05.

Commit message

Add device pixel ratio support. Small fix for the panel hack.

Description of the change

To test DPR 2:
stop unity8
initctl set-env --global QT_DEVICE_PIXEL_RATIO=2
start unity8

* Are there any related MPs required for this MP to build/function as expected? Please list.
Nothing depends on this directly, but to do a full test, you'll need:
https://code.launchpad.net/~unity-team/qtmir/dpr/+merge/257514

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Y
 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:261
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/204/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-vivid-armhf-ci/30
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-vivid-armhf-ci/30/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/204/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:262
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/205/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-vivid-armhf-ci/31
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-vivid-armhf-ci/31/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/205/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

In src/ubuntumirclient/window.cpp

"""
    // Window manager can give us a final size different from what we asked for
    // so let's check what we ended up getting
    {
        MirSurfaceParameters parameters;
        mir_surface_get_parameters(d->surface, &parameters);

        geometry.setWidth(divideAndRoundUp(parameters.width, devicePixelRatio()));
        geometry.setHeight(divideAndRoundUp(parameters.height, devicePixelRatio()));

        DLOG("[ubuntumirclient QPA] created surface has pixel size (%d, %d) and device-pixel size (%d, %d)",
                parameters.width, parameters.height, geometry.width(), geometry.height());
    }

    // Assume that the buffer size matches the surface size at creation time
    d->bufferSize = geometry.size();
"""

Here bufferSize will be initialized with a scaled size, but it should have the real size.
So bufferSize initialization should probably moved above the line "// Window manager can give us a final size different[...]".

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:264
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/206/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-vivid-armhf-ci/32
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-vivid-armhf-ci/32/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/206/rebuild

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

@Daniel, you were correct, I had mixed up a case where pixel != devicePixel. Since this is far too easy to screw up, I thought it a good idea to append "Px" to any variable that is in pixels, and so any other geometry is in device pixels. I think it helps distinguish them.

Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

In src/ubuntumirclient/window.cpp:

"""
    DLOG("[ubuntumirclient QPA] creating surface at (%d, %d) with pixel size (%d, %d) with title '%s'\n",
            d->bufferSizePx.x(), d->bufferSizePx.y(), d->bufferSizePx.width(), d->bufferSizePx.height(), title.data());
"""

bufferSizePx has no x() and y() (as it's a QSize, not a QRect). I think code won't build if you enable debug logging?

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:265
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/207/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-vivid-armhf-ci/33
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-vivid-armhf-ci/33/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/207/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:266
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/214/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/5
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/5/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/214/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:268
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/223/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/14/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/223/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:269
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/224/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/15/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/224/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:270
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/225/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/16/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/225/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:271
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/226/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/17
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/17/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/226/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:272
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/227/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/18
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/18/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/227/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:273
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/239/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/30
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/30/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/239/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:274
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/241/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/32
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/32/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/241/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:275
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/244/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/35
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/35/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/244/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:276
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/245/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/36
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/36/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/245/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:277
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/259/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/50
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/50/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/259/rebuild

review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:278
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/260/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/51
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/51/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/260/rebuild

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

Ok, should be good to go

Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

Looks good to me (didn't test yet).

review: Approve (code)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:280
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/262/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/53
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/53/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/262/rebuild

review: Approve (continuous-integration)
Revision history for this message
Daniel d'Andrada (dandrader) wrote : Posted in a previous version of this proposal

And it works fine as well.

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

Why is that function in src/ubuntumirclient/utils.h inside an anonymous namespace?

review: Needs Information
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:290
http://jenkins.qa.ubuntu.com/job/qtubuntu-ci/289/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/80
        deb: http://jenkins.qa.ubuntu.com/job/qtubuntu-wily-armhf-ci/80/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtubuntu-ci/289/rebuild

review: Approve (continuous-integration)
Revision history for this message
Gerry Boland (gerboland) wrote :

> Why is that function in src/ubuntumirclient/utils.h inside an anonymous
> namespace?

To stop it polluting the global namespace. The header file is private.

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

Why did you remove that chunk from UbuntuSurface constructor in src/ubuntumirclient/window.cpp? In any case that removal is unrelated to DPR work.

"""
        auto geom = mWindow->geometry();
        geom.setWidth(parameters.width);
        geom.setHeight(parameters.height);
        if (mWindowState == Qt::WindowFullScreen) {
            geom.setY(0);
        } else {
            geom.setY(panelHeight());
        }
"""

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

On 17/12/2015 11:52, Daniel d'Andrada wrote:
> Why did you remove that chunk from UbuntuSurface constructor in src/ubuntumirclient/window.cpp? In any case that removal is unrelated to DPR work.
>
> """
> auto geom = mWindow->geometry();
> geom.setWidth(parameters.width);
> geom.setHeight(parameters.height);
> if (mWindowState == Qt::WindowFullScreen) {
> geom.setY(0);
> } else {
> geom.setY(panelHeight());
> }
> """

And so is the panel hack refactoring you're doing.

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

> Why did you remove that chunk from UbuntuSurface constructor in
> src/ubuntumirclient/window.cpp? In any case that removal is unrelated to DPR
> work.
>
> """
> auto geom = mWindow->geometry();
> geom.setWidth(parameters.width);
> geom.setHeight(parameters.height);
> if (mWindowState == Qt::WindowFullScreen) {
> geom.setY(0);
> } else {
> geom.setY(panelHeight());
> }
> """

It was a bit related. I wanted to prevent UbuntuSurface having any knowledge of device-indepedent pixels, I wanted it to be purely pixel based. UbuntuSurface pretty much a wrapper for a MirSurface. So that small refactor allowed me to do that, plus eliminate a small bit of duplicate code.

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

Panel hack refactoring was to ensure the hack was applied at UbuntuWindow creation in a correct fashion.

The issue I found with the old hack was if window was created in fullscreen state, with y=0, then
  if (state == Qt::WindowFullScreen && geometry().y() != 0) {
was not entered, but
  } else if (geometry().y() == 0) {
was. So fullscreen surface got the panel hack applied.

I fixed it here as I was testing widget-based apps which did this.

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

Code looks ok

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

Works fine.

review: Approve
lp://qastaging/~unity-team/qtubuntu/DPR updated
291. By Gerry Boland

Merge loggingCategory with merged trunk

292. By Gerry Boland

Bump changelog date to suit

Unmerged revisions

292. By Gerry Boland

Bump changelog date to suit

291. By Gerry Boland

Merge loggingCategory with merged trunk

290. By Gerry Boland

Bad merge, fixing

289. By Gerry Boland

Merge updated loggingCategory

288. By Gerry Boland

Merge Daniel's categroy logging branch

287. By Gerry Boland

Fix bug in panel-height hack where fullscreen surface could actually have the hack enabled. Also do little renaming & typo fix

286. By Gerry Boland

Fix surfaces positioning when going to & from fullscreen

285. By Gerry Boland

Fix camera app sizing - only tell qt about geometry change after mir replies, do not assume it will occur

284. By Gerry Boland

Typo making initial surface geometry incorrect

283. By Gerry Boland

Merge trunk

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