Merge lp://qastaging/~smspillaz/compiz-core/compiz-core.decor_synchronization into lp://qastaging/compiz-core/0.9.8

Proposed by Sam Spilsbury
Status: Merged
Approved by: Sam Spilsbury
Approved revision: 3024
Merged at revision: 3131
Proposed branch: lp://qastaging/~smspillaz/compiz-core/compiz-core.decor_synchronization
Merge into: lp://qastaging/compiz-core/0.9.8
Diff against target: 1457 lines (+849/-66)
15 files modified
gtk/window-decorator/decorator.c (+48/-24)
gtk/window-decorator/events.c (+12/-1)
gtk/window-decorator/gtk-window-decorator.c (+9/-0)
gtk/window-decorator/gtk-window-decorator.h (+8/-0)
gtk/window-decorator/wnck.c (+13/-11)
include/decoration.h (+18/-0)
libdecoration/decoration.c (+86/-0)
plugins/decor/CMakeLists.txt (+3/-1)
plugins/decor/src/decor.cpp (+99/-22)
plugins/decor/src/decor.h (+35/-7)
plugins/decor/src/pixmap-requests/CMakeLists.txt (+62/-0)
plugins/decor/src/pixmap-requests/include/pixmap-requests.h (+188/-0)
plugins/decor/src/pixmap-requests/src/pixmap-requests.cpp (+93/-0)
plugins/decor/src/pixmap-requests/tests/CMakeLists.txt (+15/-0)
plugins/decor/src/pixmap-requests/tests/pixmap-requests/src/test-decor-pixmap-requests.cpp (+160/-0)
To merge this branch: bzr merge lp://qastaging/~smspillaz/compiz-core/compiz-core.decor_synchronization
Reviewer Review Type Date Requested Status
Daniel van Vugt Needs Resubmitting
Alan Griffiths Pending
Review via email: mp+103444@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-04-25.

Description of the change

Add synchronization primitives to the decoration protocol so that there isn't a race where we bind a texture that's being freed.

Things changed

 * update_window_decoration_size made semi-private, users should use request_update_window_decoration_size instead, which prepares for a pixmap update and sends a message to the decorated process that a new pixmap is waiting.
 * decor_post_pending : tells the decorated process that a new decoration is pending
 * decor_post_request : tells the decorator that a new pixmap can be drawn to
 * decor_post_delete_pixmap : tells the decorator that a pixmap is no longer in use by the decorated process and can be deleted by the server

I also did some necessary refactoring so we could get this under test, namely:

DecorPixmap class (implements DecorPixmapInterface): a simple RAII class which ensures that a DecorPixmapDeletionInterface is invoked to delete the server side pixmap when the DecorPixmap class is destructed

X11DecorPixmapReceiver class (implements DecorPixmapReceiverInterface): a per-decoration owned class which tracks the update state of the pixmap owned by the decoration from the decorator, calls through to appropriate methods on a DecorPixmapRequestorInterface to request new pixmaps (ensuring that chattyness is reduced to when a) the decorator tells us that a new pixmap is ready and b) when a new pixmap was already pending and we need to fetch the newest one)

X11DecorPixmapRequestor class (implements DecorPixmapRequestorInterface): a per-window owned class which handles dispatch to the decorator (eg decor_post_pending) and requests from the decorator in _COMPIZ_DECOR_PENDING messages to communicate when decorations should be redrawn at new sizes

X11PixmapDeletor class (implements DecorPixmapDeletionInterface): a per-pixmap class which sends decor_post_delete_pixmap to the decorator when a pixmap is no longer in use by the decor plugin

DecorationListFindMatchingInterface: an interface implemented by DecorationList to find a decoration matching a specified criteria

DecorationInterface: an interface implemented by Decoration to get the receiver and properties about that decoration.

Tests included (Google Mock)

To post a comment you must log in.
Revision history for this message
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

I haven't reviewed this, but please fix and/or resubmit to resolve the conflicts.

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

For starters, there are non-trivial conflicts that need fixing.

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

I've linked the two related bugs that I think this branch is designed to fix. It seems to.

Regression: Active windows often get inactive title bar decorations. For example, if I open a new terminal (Ctrl+Shift+N) then the new window gets activated and focused (and is usable), but keeps inactive decorations. There's not a single window on my screen with active window decorations now.

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

On Tue, 24 Apr 2012, Daniel van Vugt wrote:

> Review: Needs Fixing
>
> I've linked the two related bugs that I think this branch is designed to fix. It seems to.
>
> Regression: Active windows often get inactive title bar decorations. For
example, if I open a new terminal (Ctrl+Shift+N) then the new window gets
activated and focused (and is usable), but keeps inactive decorations.
There's not a single window on my screen with active window decorations now.\\\

I did not see this on my system. However, maybe it would be worth
requesting a pixmap redraw whenever the active state changes.

> --
> https://code.launchpad.net/~smspillaz/compiz-core/compiz-core.decor_synchronization/+merge/102801
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.decor_synchronization.
>

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

I'll deal with this a little later.

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

Looks OK, works OK. As much as I would like to see compiz get smaller and simpler, I am very much liking the outcome of this branch.

And as far as I can tell, the header changes don't *seem* to break the ABI in a way that affects other packages/projects.

review: Approve
Revision history for this message
Sam Spilsbury (smspillaz) wrote :

> Looks OK, works OK. As much as I would like to see compiz get smaller and
> simpler, I am very much liking the outcome of this branch.
>
Look to simplicity of execution, not simplicity of code. Refactoring to make code testable may increase code size, but it doesn't increase complexity.

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

Please resubmit this branch. I just reverted it from lp:compiz-core because triplesquarednine found lots of crashes and bisected, finding this branch was the cause.

review: Needs Resubmitting

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