Merge lp://qastaging/~vanvugt/unity/fix-918329-trunk into lp://qastaging/unity

Whoops: https://code.launchpad.net/~smspillaz/unity/unity.fix_881190_918360/+merge/89181

Alright - so what do you think then? I'm in favor of using smart pointers where possible to do so, obviously we must use raw pointers in the list, but the list is managed by the destructor of the minimized window handler.

(I really need to clean up that file, but then again, the file probably represents how much of a hack fake minimization really is, which is the only way to get live previews from minimized windows *sigh*)

I tried an in-between solution as you suggest, where the list is raw pointers and the share_ptrs stay in unityshell. It worked, however that necessitated some ugly dynamic typecasts. I think this version is better because the types are cleanly matched without casting required.

Smart pointers probably have their place. But in this case they caused leaks and made the code much harder to read and maintain.

P.S. One of us just wasted a day because you forgot to mark the bug in progress or even assign it to yourself :(

On Fri, 20 Jan 2012, Daniel van Vugt wrote:

> I tried an in-between solution as you suggest, where the list is raw pointers and the share_ptrs stay in unityshell. It worked, however that necessitated some ugly dynamic typecasts. I think this version is better because the types are cleanly matched without casting required.
>
> Smart pointers probably have their place. But in this case they caused leaks and made the code much harder to read and maintain.

There is no need for dynamic casting as there is only a single line of
inheritance.

I'll merge this one though.

> --
> https://code.launchpad.net/~vanvugt/unity/fix-918329-trunk/+merge/89391
> You are requested to review the proposed merge of lp:~vanvugt/unity/fix-918329-trunk into lp:unity.
>

Thanks.

And you're right about static casts instead of dynamic. But this version avoids all casts which is even better I think.

On Fri, 20 Jan 2012, Daniel van Vugt wrote:

> Thanks.
>
> And you're right about static casts instead of dynamic. But this version avoids all casts which is even better I think.

Yes, boost::static_pointer_cast has the cost of the boost::shared_ptr
constructor (which is not that much, but something to dislike anyways).

I don't know why the static pointer casts were changed to dynamic casts.
That seems excessively silly, especially considering that one of them was
in a paint function (!). However, use of downcasting usually indicates a
problem with the design, one that should be revisited later (also related
to testing, since the base class exists for testing purposes)

> --
> https://code.launchpad.net/~vanvugt/unity/fix-918329-trunk/+merge/89391
> You are reviewing the proposed merge of lp:~vanvugt/unity/fix-918329-trunk into lp:unity.
>