Compiz

Merge lp://staging/~aacid/compiz/do_not_change_viewport_on_resize into lp://staging/compiz/0.9.9

  1. do_not_change_viewport_on_resize
  2. Merge into 0.9.9
Proposed by Sam Spilsbury
Status: Merged
Approved by: Daniel van Vugt
Approved revision: 3363
Merged at revision: 3417
Proposed branch: lp://staging/~aacid/compiz/do_not_change_viewport_on_resize
Merge into: lp://staging/compiz/0.9.9
Diff against target: 84 lines (+9/-15)
1 file modified
src/window.cpp (+9/-15)
To merge this branch: bzr merge lp://staging/~aacid/compiz/do_not_change_viewport_on_resize
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
jenkins (community) continuous-integration Approve
Sam Spilsbury Needs Information
Review via email: mp+129089@code.staging.launchpad.net

This proposal supersedes a proposal from 2012-09-28.

Commit message

Do not move windows between viewports/workspaces when changing its size
(e.g. maximizing). (LP: #776435)

.

Description of the change

Do not move windows between viewports/workspaces when changing its size (e.g. maximizing)

To be honest i'm a bit "scared" since this touches a function used by lots of other functions, but if we never want a window to change to a different viewports/workspaces i guess it makes sense.

To post a comment you must log in.
Revision history for this message
jenkins (martin-mrazik+qa) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:3362
http://jenkins.qa.ubuntu.com/job/compiz-ci/64/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/64/console

review: Approve (continuous-integration)
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Hmm, I think the intention of this code is to prevent windows that maximize themselves from going on to other viewports. I think it might make sense to add a check here to check if the window is on the currently active viewport, and if so, to always maximize it on that viewport.

Do you think you might be able to get this code under test?

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Actually, I'm not sure about the validity of the bug itself. The bug seems to be that windows get maximized on the workspace which contains their centre-point. That's the way it was designed and the way it works. But that's not what's always intuitive to the user and hence the bug.

If that's the functionality we're aiming to change here then it looks like the wrong code is being modified.

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

I think you should look at this instead:

screen.cpp: compiz::private_screen::viewports::viewportForGeometry

review: Needs Fixing
Revision history for this message
Albert Astals Cid (aacid) wrote : Posted in a previous version of this proposal

Daniel, right, the code is doign what you say (maximize on the workspace which contains their centre-point) which both I and Sam (that set the bug to Confirmed) think does not make sense.

I disagree the wrong code is being modified, what
   CompScreenImpl::viewportForGeometry (const CompWindow::Geometry& gm, CompPoint& viewport)
does is not return in viewport the viewport that gm should have but the "difference" in viewports from the current one, i.e. it can return {-1, -1} if it thinks the window shall be one viewport left and one top from its current one.

Of course I could make that function return always {0,0} (i.e. don't move me) but that might potentially break other plugins that use it like the group and the wall plugins

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

OK, I have no opinion until I can review this in detail later.

review: Abstain
Revision history for this message
Albert Astals Cid (aacid) wrote : Posted in a previous version of this proposal

FWIW: I'm going on holiday tomorrow, will be back on 1st October

Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

The branches have changed. Please resubmit targeting lp:compiz

review: Needs Resubmitting
Revision history for this message
jenkins (martin-mrazik+qa) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:3362
http://jenkins.qa.ubuntu.com/job/compiz-ci/139/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/139/console

review: Needs Fixing (continuous-integration)
Revision history for this message
jenkins (martin-mrazik+qa) wrote : Posted in a previous version of this proposal

FAILED: Continuous integration, rev:3363
http://jenkins.qa.ubuntu.com/job/compiz-ci/169/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/169/console

review: Needs Fixing (continuous-integration)
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

I don't think I'll have time in the foreseeable future to get this under test. Are we happy to merge it without tests? What was the result from manual testing?

review: Needs Information
Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:3363
http://jenkins.qa.ubuntu.com/job/compiz-ci/173/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/173/console

review: Approve (continuous-integration)
Revision history for this message
Albert Astals Cid (aacid) wrote :

I have done limited manual testing and it works as I would expect it to work, but that is of course not much :D

As for unit testing if you give me some instructions as of how you'd like this unit tested i can give it a try.

Otherwise we can try to merge it to a R-only branch and let very early adopters do the testing?

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Reviewed and tested again. It looks correct and fixes the bug.

Although the relevant logic is a prime candidate for automated testing, the relevant classes are not yet isolated enough to be unit-testable. And doing so in src/window.cpp is a risky proposition.

review: Approve

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