Merge lp://qastaging/~smspillaz/compiz-core/compiz-core.fix_969108.2 into lp://qastaging/compiz-core

Again, I can't endorse a major rewrite of critical functionality this late.

Also, this 1000 lines of code seems to be aimed at solving a problem that I planned on solving myself in only a dozen or so lines. Is it really necessary to introduce such vast complexity?

1) This isn't a rewrite
2) The only functional changes are in the form of

42 if (state & CompAction::StateCancel)
43 - ms->w->move (ms->savedX - ms->w->geometry ().x (),
44 - ms->savedY - ms->w->geometry ().y (), false);
45 + ms->addMovement (MoveWindow::get (ms->w), CompPoint (ms->savedX - ms->w->geometry ().x (),
46 + ms->savedY - ms->w->geometry ().y ()));
47
48 ms->w->syncPosition ();
49
50 @@ -484,8 +486,7 @@
51
52 if (dx || dy)
53 {
54 - w->move (wX + dx - w->geometry ().x (),
55 - wY + dy - w->geometry ().y (), false);
56 + ms->addMovement (MoveWindow::get (w), CompPoint (dx, dy));
62
63 bool
64 +MoveWindow::doRelease (const CompPoint &d)
65 +{
66 + window->move (d.x (), d.y (), false);
67 + return true;
68 +}
69 +
70 +void
71 +MoveScreen::dispatchDamage (compiz::queues::QueueReleaseInterface *releasable)
72 +{
73 + MoveWindow *mw = static_cast <MoveWindow *> (releasable);
74 + mw->cWindow->addDamage ();
75 +}
76 +
77 +bool
78 MoveScreen::registerPaintHandler(compiz::composite::PaintHandler *pHnd)
79 {
80 hasCompositing = true;
81 @@ -677,9 +692,27 @@
82 MoveScreen::unregisterPaintHandler()
83 {
84 hasCompositing = false;
85 + queueLock.reset ();
86 cScreen->unregisterPaintHandler ();
87 }
88
89 +void
90 +MoveScreen::preparePaint (int ms)
91 +{
92 + if (queueLock)
93 + queueLock->unlock ();
94 +
95 + cScreen->preparePaint (ms);
96 + cScreen->preparePaintSetEnabled (this, false);
97 +}
98 +
99 +void
100 +MoveScreen::addMovement (MoveWindow *mw, const CompPoint &d)
101 +{
102 + cScreen->preparePaintSetEnabled (this, true);
103 + pendingMovements->addPending (d, mw);
104 +}
105 +
113 - yConstrained (false)
114 + yConstrained (false),
115 + pendingMovementsStorage (new compiz::queues::impl::AccumulatedMapStorage <CompPoint> ()),
116 + pendingMovements (new compiz::queues::impl::Queue <CompPoint> (boost::shared_static_cast <compiz::queues::QueueStorageInterface <CompPoint> > (pendingMovementsStorage)))
123 CompositeScreenInterface::setHandler (cScreen);
124 + queueLock = pendingMovements->obtainLock (boost::bind (&MoveScreen::dispatchDamage, this, _1));
125 + cScreen->preparePaintSetEnabled (this, false);

That's fairly small

3) The reason the diff is so large is because 100 of 1200 lines are cmake and 600 of the 1200 lines are tests
4) This solution is aimed at making releasable queues more generic
5) Its blocking other work

Plenty of good reasons to merge this before precise.

Making destructors pure virtual is only really necessary if there are no other pure virtual methods and you want to make the class abstract. Other than that they are just a PITA.

I don't know the compiz coding standards, but for abstract code like this I'd expect at the very least 2 comments: 1) In queue.cpp/h explaining what the general idea behind all the interfaces are, 2) in the move.cpp explaining what it is using the clever queues for (ie. why a bog standard list is not good enough)

It isn't obvious what contracts the template interfaces QueueStorageInterface, QueueInterface, and QueueLockInterface represent. E.g. I'd expect a lock to have "lock()" and "unlock()" methods or similar. Do they need to be templates?

The pure destructors and NVPI clutter up the code.

In MoveScreen the new variables pendingMovementsStorage, pendingMovements and queueLock could be private. (I suspect others could too - what is it with compiz and public data members?)

I don't see why pendingMovementsStorage is a member variable - it's only use is in initalizing pendingMovements. In any case, it should be of the usage type - shared_ptr<QueueStorageInterface> not the implementation type.

There is no need for pendingMovements to be allocated on the heap instead of being a member variable. (The same change for pendingMovementsStorage which would require a signature change for compiz::queues::impl::Queue::Queue - but that's minor.)

> It isn't obvious what contracts the template interfaces QueueStorageInterface,
> QueueInterface, and QueueLockInterface represent. E.g. I'd expect a lock to
> have "lock()" and "unlock()" methods or similar. Do they need to be
> templates?

QueueReleaseInterface, QueueInterface and QueueStorageInterface need to know the type that is being stored. As such, they are templates.

QueueLockInterface takes a lock on a QueueUnlockableInterface on construction. Eg, the very act of constructing a QueueLockInterface means that it will receive callbacks and block dispatching of the queue items until the client calls unlock () on it, at which point will call a method in QueueUnlockableInterface to decrement the lock count.

QueueStorageInterface is the abstraction of how queued items are stored.

QueueInterface is the interface to the Queue itself, eg, pushing data into the queue.

>
> The pure destructors and NVPI clutter up the code.
>
> In MoveScreen the new variables pendingMovementsStorage, pendingMovements and
> queueLock could be private. (I suspect others could too - what is it with
> compiz and public data members?)

Ack, although I believe we're using this from MoveWindow.

>
> I don't see why pendingMovementsStorage is a member variable - it's only use
> is in initalizing pendingMovements. In any case, it should be of the usage
> type - shared_ptr<QueueStorageInterface> not the implementation type.

You are correct.

I believe the reason I made it a member was merely for convenience, without that, this line

pendingMovements (new compiz::queues::impl::Queue <CompPoint> (boost::shared_static_cast <compiz::queues::QueueStorageInterface <CompPoint> > (pendingMovementsStorage)))

would look something like

pendingMovements (new compiz::queues::impl::Queue <CompPoint> (boost::shared_static_cast <compiz::queues::QueueStorageInterface <CompPoint> > (compiz::queues::impl::AccumulatedMapStorage <CompPoint>::Ptr ((new compiz::queues::impl::AccumulatedMapStorage <CompPoint> ()))))

(which is a bit of a monstrosity :( )

>
> There is no need for pendingMovements to be allocated on the heap instead of
> being a member variable. (The same change for pendingMovementsStorage which
> would require a signature change for compiz::queues::impl::Queue::Queue - but
> that's minor.)

I think the reason why I had it on the heap was because if it was on the stack then this would mean we'd pass QueueStorageInterface by const reference and we'd have to copy the temporary into the member variable inside of ::Queue . That would mean adding copying semantics to something which shouldn't really be copied anyways (QueueStorageInterface describes /how/ the queue data should be stored, it isn't a value type in itself).

pendingMovements on the other hand could be on the stack.

> I don't know the compiz coding standards, but for abstract code like this I'd
> expect at the very least 2 comments: 1) In queue.cpp/h explaining what the
> general idea behind all the interfaces are, 2) in the move.cpp explaining what
> it is using the clever queues for (ie. why a bog standard list is not good
> enough)

Yes, I will add such comments. Thanks Mikkel

> Making destructors pure virtual is only really necessary if there are no other
> pure virtual methods and you want to make the class abstract. Other than that
> they are just a PITA.

Ack

> > It isn't obvious what contracts the template interfaces
> QueueStorageInterface,
> > QueueInterface, and QueueLockInterface represent. E.g. I'd expect a lock to
> > have "lock()" and "unlock()" methods or similar. Do they need to be
> > templates?
>
> QueueReleaseInterface, QueueInterface and QueueStorageInterface need to know
> the type that is being stored. As such, they are templates.

Yes, but they are only instantiated on one type in real code. (And another in the tests.)

> >
> > The pure destructors and NVPI clutter up the code.
> >
> > In MoveScreen the new variables pendingMovementsStorage, pendingMovements
> and
> > queueLock could be private. (I suspect others could too - what is it with
> > compiz and public data members?)
>
> Ack, although I believe we're using this from MoveWindow.

Not in the code I see.

> > I don't see why pendingMovementsStorage is a member variable - it's only use
> > is in initalizing pendingMovements. In any case, it should be of the usage
> > type - shared_ptr<QueueStorageInterface> not the implementation type.
>
> You are correct.
>
> I believe the reason I made it a member was merely for convenience, without
> that, this line
>
> pendingMovements (new compiz::queues::impl::Queue <CompPoint>
> (boost::shared_static_cast <compiz::queues::QueueStorageInterface <CompPoint>
> > (pendingMovementsStorage)))
>
> would look something like
>
> pendingMovements (new compiz::queues::impl::Queue <CompPoint>
> (boost::shared_static_cast <compiz::queues::QueueStorageInterface <CompPoint>
> > (compiz::queues::impl::AccumulatedMapStorage <CompPoint>::Ptr ((new
> compiz::queues::impl::AccumulatedMapStorage <CompPoint> ()))))

Or without the pointless cast...

pendingMovements (new compiz::queues::impl::Queue <CompPoint> (compiz::queues::impl::QueueStorageInterface <CompPoint>::Ptr ((new compiz::queues::impl::AccumulatedMapStorage <CompPoint> ())))

And if you don't repeat compiz::queues::impl:: all the time.

pendingMovements (new cqi::Queue <CompPoint> (cqi::QueueStorageInterface <CompPoint>::Ptr ((new cqi::AccumulatedMapStorage <CompPoint> ())))

> > There is no need for pendingMovements to be allocated on the heap instead of
> > being a member variable. (The same change for pendingMovementsStorage which
> > would require a signature change for compiz::queues::impl::Queue::Queue -
> but
> > that's minor.)
>
> I think the reason why I had it on the heap was because if it was on the stack
> then this would mean we'd pass QueueStorageInterface by const reference and
> we'd have to copy the temporary into the member variable inside of ::Queue .
> That would mean adding copying semantics to something which shouldn't really
> be copied anyways (QueueStorageInterface describes /how/ the queue data should
> be stored, it isn't a value type in itself).

I think there's a misunderstanding. But that's moot in view of the above discussion.

> pendingMovements on the other hand could be on the stack.

A data member. Then the "monsterous" line becomes:

pendingMovements (cqi::QueueStorageInterface <CompPoint>::Ptr ((new cqi::AccumulatedMapStorage <CompPoint> ()))