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> ()))

Code cleaned up. Resubmitted ... though it looks like I have more to do. Fun.

> 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.)

I plan to use this in other parts of the code. It was almost generic as it was, and is quite useful as a separate mechanism IMO

Will use namespace aliases as recommended

Code cleaned up and resubmitted again

Seems to work. And the functionality seems a good idea.

But, now we can read what they do, the names of the queues classes seem a bit awkward...

"QueueInterface" does only a small subset of what I'd expect a queue to do and adding "Interface" to the name seems verbose (not that I like Microsoft's "I" prefix either). "Appender" may be better?

"QueueLockInterface" this doesn't behave as I expect from a lock. It is a pre-release callback to request permission to release the queue.

(I find the other names problematic too - but don't want to go on too much.)

On the design side, I don't like the fact that downcasts are required by the code using these classes. As a general principle templates should allow this to be avoided by propagating type information.

> Seems to work. And the functionality seems a good idea.
>
> But, now we can read what they do, the names of the queues classes seem a bit
> awkward...
>
> "QueueInterface" does only a small subset of what I'd expect a queue to do and
> adding "Interface" to the name seems verbose (not that I like Microsoft's "I"
> prefix either). "Appender" may be better?

The I/Interface/Whatever discussion needs to be had somewhere else.

>
> "QueueLockInterface" this doesn't behave as I expect from a lock. It is a
> pre-release callback to request permission to release the queue.

QueueHoldInterface ?

>
> (I find the other names problematic too - but don't want to go on too much.)
>
> On the design side, I don't like the fact that downcasts are required by the
> code using these classes. As a general principle templates should allow this
> to be avoided by propagating type information.

550 + mItems[static_cast <QueueReleaseInterface *> (i)] += t;
551 +
552 + return true;
553 + }
554 +
555 + void doRelease ()
556 + {
557 + typename std::map <QueueReleaseInterface *, T>::iterator it = mItems.begin ();
558 +
559 + for (; it != mItems.end (); it++)
560 + {
561 + QueueReleasableInterface <T> *releasable = static_cast <QueueReleasableInterface <T> *> (it->first);

These two are avoidable. I'll try something else. As for the downcast to MoveWindow, it should be possible to avoid that one by changing the boost::function to another interface like a QueueItemReadyCallback or perhaps adding an notifyItemReady (); to QueueReleaseInterface