On Fri, Oct 7, 2011 at 6:50 AM, Tim Penhey <email address hidden> wrote:
> Review: Approve
>
> Hi Sam,
>
> This class seems weird.  Ideally we should try to avoid
> global scope as much as possible, which includes static lists
> if at all possible.
>

Yep, it is :)

The lists aren't in the global scope though, they're in the namespace
for the CompizMinimizedWindowHandler class.

> OMG... where to start with this.  In order to gain context
> about what was going on, I opened the complete file.  That needs
> to be fixed badly.  There is a template class for absolutely
> no reason, apart from adding complexity where we don't need it.

Yes, I know it's ugly and complicated as sin. I was cursing myself as
I wrote it, trust me. The whole reason it's that complicated is
because I wanted to have something that we could just drop-in and
drop-out of unityshell when the time was right. I initially intended
to put this inside of the workarounds plugin, but I can't break the
ABI, so I had to port over all this functionality to unityshell.
Because it's quite complicated functionality that ties into a lot of
pieces of core, I'm using templates as a means to sort of bridge the
gap, so the class can be self-contained and yet still hook into a lot
of what core does. I also wanted to keep it as self-contained as
possible so that we could test it externally. For precise this is
going to be removed and implemented properly inside the workarounds
plugin where it belongs.

So, there are reasons for everything ;-)

>
> It is accessing a global variable called "screen" AFAICS.
> Please tell me I'm wrong.

Yes, that's part of compiz.

>
> We have one class with two responsibilities.  The static members
> control the static data, and the member functions control the
> minimisation of the window itself.  This really needs to be
> broken into two, kill the static methods, have the object
> owned by the UnityScreen.

Is there anything particularly wrong with that paradigm? I can make
another class like a MinimizedWindowManager, but it seemed to make
more sense to just keep static data for the template instantiation,
since we need to keep track of which windows we have applied fake
minimization to in order to update their wrap functions and internal
state in core.

>
> Also... why are these in the compiz namespace?  That is just nuts.
>

See above

> About a gazillion lines of complicated code can be made simpler
> here.
>

See above +1

> While this branch may well fix the bug, it doesn't make me happy.
>

Neither :) The thing it actually does is more or less an elaborate lie
to applications so that they'll continue to be visible while believing
that they are minimized. There's no proper interface for achieving
this goal in either X or the EWMH so it is basically just a giant
self-contained hack. Of course, I think that no matter how you
implemented this code, even if you put it in it's own plugin and
exported the controller objects for it (which is exactly what I intend
to do ... once I'm allowed to), it would always look ugly.

> --
> https://code.launchpad.net/~smspillaz/unity/unity.fix_864758/+merge/78251
> You are the owner of lp:~smspillaz/unity/unity.fix_864758.
>



-- 
Sam Spilsbury