Code review comment for lp:~canonical-dx-team/unity/unity.qlmenuitems

Good stuff, the tests look good and it runs, however I have some comments that I'd like to see addressed before approving:

- There's a conflict in po/unity.pot

- In + else
64 + {
65 + QuicklistMenuItemLabel* item = new QuicklistMenuItemLabel (newitem, NUX_TRACKER_LOCATION);
66 + quicklist->AddMenuItem (item);
67 + }

  I think you should explicitly check for label item, and in the "else" you should warn that we don't support item of type $type

- "// enable this once all proper QuicklistMenuitems are fully implemented" why is this code this here?

- You don't need #if defined (NUX_OS_LINUX), it's all we care about

- GetDPI () should get the DPI from GtkSettings

- Do const gchar * checks against NULL, not against 0. Return NULL for a method returning a char pointer, don't return 0

- QuicklistMenuItem::GetTextExtents and QuicklistMenuItem::UpdateTexture, do they need to be in trunk if they are commented out?

- Why do you need to cache _enabled & _active in QuicklistMenuItem?

- QuicklistMenuItem () with the debug constructor doesn't connect to thee menuitem's signals, so it won't stay up-to-date...

- Why aren't sub-classes using GetLabel() instead of getting information from dbusmenu themselves?

- Tons of commented out lines, if we dont' need them can we drop them?

- QuicklistMenuItemCheckmark::DrawText leaking Leaking PangoContext?

- Do we have multiple GetTextExtents? Should they not be in a utility file?

- What is SetFontWeight/Style etc for? Why are exposing this publicly right now?

- Waaay too much copy-and-pasted code, needs to be moved in to a Utils.h/cpp for clarity

- I *really* don't like having to use NStrings everywhere. dbusmenu is handing things to us as const gchar* and that's what we need to feed into cairo/pango, so why do we add NString in between? I'd like this changed to const char* like the rest of the plugin.

- Tests look great!