Code review comment for lp:~sil2100/unity-2d/trunk.lp839628

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

> Looks great, Łukasz!
>
> Just a couple of things:
> - Remove the addition of an empty line in appnameapplet.cpp:142

Will do that!

> - One little change that might help keep the code simple: remove the entries()
> method from IndicatorsWidget, and instead add a method "showFirstMenu()" which
> you can call from both filters.

hm, this might be a problem. Because indeed, the entries() method is called in both filters, but in the IndicatorApplet it is called from an IndicatorsWidget object, where in AppNameApplet it is called from an MenuBarWidget object - where both these classes are just derivatives of QWidget. So just adding a showFirstMenu() instead would not really decrease the code size, since I would have to add the same method doing the same in 2 classes. In fact, I had a method like this in the beginning (when I only handled application menus), but dropped it because such a method has potentially only one use - to show the menu. Where an entries() method can be used in the future for other (yet unplanned) things.

I could, of course, create a class - for instance, WidgetWithIndicators, that would simply be a QWidget with the showFirstMenu()/entries() method, but I think that would be unnecessary bloating of the namespace.

> - Is the setOpened() method really needed? Why doesn't the indicator emit the
> entryActivated() signal in this case?

I just noticed that without it, the menubar acts strangely i.e. the application menu appears in a random place. I think it's because of the fact, that the updateWidgets() method that shows the menu bar and ensures that it's properly adjusted gets always called before the entryActivated() signal. And we need the m_isOpened to be true before the updateWidgets() call, otherwise the behavior is wrong.

unity-2d-panel: [DEBUG] eventFilter()
unity-2d-panel: [DEBUG] updateWidgets()
unity-2d-panel: [DEBUG] entryActivated()

So I though that instead of fighting with this, it would be best just to create that method and call it beforehand.

« Back to merge proposal