Merge lp://qastaging/~agateau/unity-2d/unity-core into lp://qastaging/unity-2d

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

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

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

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.

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

On Tue, 12 Jul 2011 21:12:39 you wrote:
> > 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.

You could say the same about unity, but I'm pushing for "namespace unity" (and
implementing).

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

No. nux::Color is in NuxCore, not Nux.

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

Sure.

> On Tue, 12 Jul 2011 21:12:39 you wrote:
> > > 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.
>
> You could say the same about unity, but I'm pushing for "namespace unity" (and
> implementing).

Not exactly, at least libunity-core is used by both unity-2d and unity-3d.

> > > 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.
>
> No. nux::Color is in NuxCore, not Nux.

Oh. Will fix.

Aurélien

lp:unity-2d/4.0 saw quite a few updates which caused a couple of conflicts. Do you mind fixing them while I do the code review?

It seems like quite a few panel bugs got fixed by this piece, nice!

I don't see any background gradient in the panel anymore, is that expected?
Also the datetime indicator is there but tiny (easy to spot by navigating with the keyboard in the panel).

> Also the datetime indicator is there but tiny (easy to spot by navigating with
> the keyboard in the panel).

This is fixed now for some reason.

* Height of 24 pixels is hardcoded in various places. It would be nice to define it only once.

* Texts in panel do not have a shadow (sunken) like in Unity 3D.
* In the indicator with the username displayed in it (is it called 'me menu'?), the icon is not vertically centered like in Unity 3D.

* In panel/applets/appname/menubarwidget.cpp:

bool MenuBarWidget::isOpened() const
{
    // FIXME
[...]

What is the FIXME about?

* Do you know why does the shared panel backend need to know about the geometry of the indicators?

Have you tested the new panel with multiple monitors? I don't have the means to do it at the moment unfortunately.

> lp:unity-2d/4.0 saw quite a few updates which caused a couple of conflicts. Do
> you mind fixing them while I do the code review?

I have a whole stack of branches right now but they are targeting trunk, not 4.0. Should I change this so they all target 4.0?

> It seems like quite a few panel bugs got fixed by this piece, nice!
>
> I don't see any background gradient in the panel anymore, is that expected?

This is fixed in the next branch (use-gtk-rendering)

> * Height of 24 pixels is hardcoded in various places. It would be nice to
> define it only once.

Yes, I got rid of a few of them in the use-gtk-rendering branch, but there are a few remainings.

> * Texts in panel do not have a shadow (sunken) like in Unity 3D.
> * In the indicator with the username displayed in it (is it called 'me
> menu'?), the icon is not vertically centered like in Unity 3D.

Both fixed in use-gtk-rendering.

> * In panel/applets/appname/menubarwidget.cpp:
>
> bool MenuBarWidget::isOpened() const
> {
> // FIXME
> [...]
>
> What is the FIXME about?

I just forgot to remove the comment. Fixed.

>
> * Do you know why does the shared panel backend need to know about the
> geometry of the indicators?

This is for accessibility support.

> Have you tested the new panel with multiple monitors? I don't have the means
> to do it at the moment unfortunately.

No I haven't. Will do.

> > lp:unity-2d/4.0 saw quite a few updates which caused a couple of conflicts.
> Do
> > you mind fixing them while I do the code review?
>
> I have a whole stack of branches right now but they are targeting trunk, not
> 4.0. Should I change this so they all target 4.0?

Please do. 4.0 is basically the branch targetting oneiric.
For this MR you can drop the prerequisite lp:~agateau/unity-2d/gtk3 as this branch cannot be merged on its own: it basically breaks the indicators more than anything else and I reviewed its code changes as part of this MR.

Once you resubmitted this one and we are sure it works as before on multi monitors setup, it's good to merge (it will need a commit message).
I am onto https://code.launchpad.net/~agateau/unity-2d/use-gtk-rendering/+merge/69113 now.

On Wed, 27 Jul 2011 23:39:33 you wrote:
> Please do. 4.0 is basically the branch targetting oneiric.
> For this MR you can drop the prerequisite lp:~agateau/unity-2d/gtk3 as this
> branch cannot be merged on its own: it basically breaks the indicators
> more than anything else and I reviewed its code changes as part of this
> MR.

Florian,

I regularly have dependent branches that can't be landed by themselves. It is
useful for review purposes. Showing particular changes in their own branch
can make reviews much easier. Merge proposals don't have to be landable by
themselves.

Launchpad knows when branches are merged, and when the subsequent merge
proposal is landed, the dependent merge proposal is marked as merged as well.

Tim