Code review comment for lp:~agateau/unity-2d/unity-core

Aurélien Gâteau (agateau) wrote :

> Hi Aurélien,
>
> I notice you have a GConnector class. It covers a very similar area to Neil's
> latest work -
> https://code.launchpad.net/~njpatel/unity/nicer-glib-signals/+merge/67439

Yes, I discussed it briefly with Neil during Platform Rally, but from what I
understand it imposes the use of sigc++ signals. We already deal with GObject
and Qt signals so I prefer not adding another signal system to our stack
(granted, the code in this branch uses sigc++ signals when we deal with
UnityCore so this may be a moot point)

> I also notice you aren't using namespaces for your top level classes - like
> the GConnector. Have you considered using one?

Not really. GConnector is in libunity-2d-private, which is not supposed to be
reused outside of unity-2d. Therefore I don't see an advantage in using
namespaces here.

> A slight niggle, <string> is in the C++ standard library, not the STL (they
> don't say we work for pedantical for nothing).

Will fix :)

> Redeclaring nux::Color is a dangerous thing. Slightly convoluted due to the
> actual nux Color being nux::color::Color, which is then used in the nux
> namespace. That is probably the only reason you are not getting linker errors
> with this.

Granted, this code is hackish. It cannot cause link errors though:
nux::Color is in Nux and we only link to NuxCore, not to Nux. I would have
used nux::Color directly instead of reimplementing it otherwise.

This code is part of a fake Cairo API used to draw the border behind the active
menubar item:

I expected the corresponding code in unity-3d to continue to evolve based on
adjustments from Design and more drawing code to be added later on. It would
have been a pain to catch up with it if I had rewritten the whole drawing code
using Qt painting API. Instead I opted to fake the Cairo API, this way I
expected to be able to catch up by simply copying the drawing code from
unity-3d.

It turned out to be a not-so-smart move in the end because by the time I was
done with my fake Cairo API, Cimi started redoing the panel drawing code using
GTK3 style API :/. I plan on investigating whether it is possible for us to
directly call the GTK3 style API to paint on QWidgets so that we can keep
rendering synchronized. At this point this whole fake Cairo code should be
scrapped.

« Back to merge proposal