Merge lp://staging/~smspillaz/compiz-core/compiz-core.fix_896586_rotate_plugin into lp://staging/compiz-core/0.9.5

Proposed by Sam Spilsbury
Status: Rejected
Rejected by: Sam Spilsbury
Proposed branch: lp://staging/~smspillaz/compiz-core/compiz-core.fix_896586_rotate_plugin
Merge into: lp://staging/compiz-core/0.9.5
Prerequisite: lp://staging/~smspillaz/compiz-core/compiz-core.fix_891591
Diff against target: 2666 lines (+1271/-684)
29 files modified
include/core/CMakeLists.txt (+0/-2)
include/core/rect.h (+0/-233)
include/core/screen.h (+6/-3)
include/core/window.h (+15/-2)
plugins/CMakeLists.txt (+1/-0)
plugins/composite/src/window.cpp (+0/-2)
plugins/decor/src/decor.cpp (+5/-16)
plugins/move/src/move.cpp (+79/-20)
plugins/move/src/move.h (+16/-3)
plugins/opengl/include/opengl/opengl.h (+9/-2)
plugins/opengl/src/paint.cpp (+27/-22)
plugins/opengl/src/window.cpp (+4/-4)
plugins/rotate/src/rotate.cpp (+20/-5)
plugins/wobbly/src/wobbly.cpp (+42/-3)
plugins/wobbly/src/wobbly.h (+2/-0)
src/CMakeLists.txt (+5/-0)
src/rect.cpp (+0/-286)
src/rect/CMakeLists.txt (+66/-0)
src/rect/include/core/rect.h (+248/-0)
src/rect/src/rect.cpp (+306/-0)
src/rect/tests/CMakeLists.txt (+31/-0)
src/rect/tests/rect/src/test-rect.cpp (+106/-0)
src/rect/tests/test-rect.cpp (+34/-0)
src/rect/tests/test-rect.h (+44/-0)
src/rect/tests/wraparound_point/src/test-rect-wraparound-point.cpp (+78/-0)
src/screen.cpp (+32/-35)
src/window.cpp (+92/-45)
src/window/geometry-saver/CMakeLists.txt (+1/-0)
src/window/geometry/CMakeLists.txt (+2/-1)
To merge this branch: bzr merge lp://staging/~smspillaz/compiz-core/compiz-core.fix_896586_rotate_plugin
Reviewer Review Type Date Requested Status
Alan Griffiths Needs Fixing
Thomi Richards Pending
Tim Penhey Pending
Review via email: mp+88493@code.staging.launchpad.net

This proposal supersedes a proposal from 2011-12-01.

Description of the change

Made the cube and rotate plugins work with paint offsets.

In order to implement this properly, we had to move from immediately updating the 2D display matrix of the window on position offset change to applying a 4x4 transformation matrix to the window before it is painted, like how global paint offsets work, which means that a lot of code was consolidated.

In terms of other plugins, the wobbly plugin needed to be updated to move its model entirely by the paint offset, since that is going to be the global display model for the window.

Added a test for points outside of rectangles to wrap-around a rectangle within it.

Moved some of the utility functions related to viewports into their own namespace.

Made CompScreen::moveViewport wrapable so that plugins can know when a viewport was just changed (eg, causing window movement to happen) rather than guessing.

Next pipe: lp:~smspillaz/compiz-core/fix_896762

To post a comment you must log in.
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote : Posted in a previous version of this proposal

ln 1204: Why the static cast? It looks to me like you're being given a CompRectTest* already...

Otherwise, looks good to me.

review: Approve
Revision history for this message
Tim Penhey (thumper) wrote : Posted in a previous version of this proposal

> CompPoint remainingVp = CompPoint (screen->vpSize ().width () - wvp.x (),
> screen->vpSize ().height () - wvp.y ());

This is an inefficient way to write:

CompPoint remainingVp (screen->vpSize ().width () - wvp.x (),
                       screen->vpSize ().height () - wvp.y ());

The first way calls the constructor, then the copy constructor, then the destructor just to initialize remainingVp.

Please just use a constructor call. Please fix declarations in compiz::viewports::wraparoundOffsetForPoint.

review: Needs Fixing
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

> > CompPoint remainingVp = CompPoint (screen->vpSize ().width () - wvp.x (),
> > screen->vpSize ().height () - wvp.y ());
>
> This is an inefficient way to write:
>
>
> CompPoint remainingVp (screen->vpSize ().width () - wvp.x (),
> screen->vpSize ().height () - wvp.y ());
>
> The first way calls the constructor, then the copy constructor, then the
> destructor just to initialize remainingVp.

I'm suprised the compiler doesn't optimize that out.

>
> Please just use a constructor call. Please fix declarations in
> compiz::viewports::wraparoundOffsetForPoint.

ack

Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

> > This is an inefficient way to write:
> >
> > CompPoint remainingVp (screen->vpSize ().width () - wvp.x (),
> > screen->vpSize ().height () - wvp.y ());
> >
> > The first way calls the constructor, then the copy constructor, then the
> > destructor just to initialize remainingVp.
>
> I'm suprised the compiler doesn't optimize that out.

It is allowed to, but not required to - so the intent is explicit with the suggested syntax. (What actually happens varies with compiler/version and context.)

The same construct exists elsewhere. Vis:

357 + CompPoint gp = CompPoint (g.pos ().x () % (screen->vpSize ().width () * screen->width ()),
358 + g.pos ().y () % (screen->vpSize ().height () * screen->height ()));
359 + CompPoint dp = gp - window->serverGeometry ().pos ();
360 CompSize dsize = CompSize (g.width () - window->serverGeometry ().width (),
361 g.height () - window->serverGeometry ().height ());
...
2242 + compiz::window::Geometry ng = w->serverGeometry ();
...
2398 + CompPoint remainingVp = CompPoint (screen->vpSize ().width () - wvp.x (),
2399 + screen->vpSize ().height () - wvp.y ());
2400 + CompPoint maxOffset = CompPoint ((remainingVp.x () * screen->width ()) -
2401 + wp.x (),
2402 + (remainingVp.y () * screen->height ()) -
2403 + wp.y ());
2404 + CompPoint minOffset = CompPoint (-((screen->vpSize ().width () * screen->width ()) - maxOffset.x ()),
2405 + -((screen->vpSize ().height () * screen->height ()) - maxOffset.y ()));
2406 + CompRect constrain = CompRect (minOffset.x (), minOffset.y (),
2407 + maxOffset.x () - minOffset.x (),
2408 + maxOffset.y () - minOffset.y ());

> > Please just use a constructor call.

+1

review: Needs Fixing
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

There still a lot on unnecessary copy-initialization

review: Needs Fixing
2958. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2959. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2960. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2961. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2962. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2963. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2964. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2965. By Sam Spilsbury

Merge and fix warnings

2966. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

Unmerged revisions

2966. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2965. By Sam Spilsbury

Merge and fix warnings

2964. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2963. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2962. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2961. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2960. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2959. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2958. By Sam Spilsbury

Merged compiz-core.fix_891591 into compiz-core.fix_896586_rotate_plugin.

2957. By Sam Spilsbury

Cleanup

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