Merge lp://qastaging/~3v1n0/unity-2d/fix-multiscreen-indicators-geometries into lp://qastaging/unity-2d

Proposed by Marco Trevisan (Treviño)
Status: Merged
Approved by: Alberto Mardegan
Approved revision: 758
Merged at revision: 759
Proposed branch: lp://qastaging/~3v1n0/unity-2d/fix-multiscreen-indicators-geometries
Merge into: lp://qastaging/unity-2d
Diff against target: 109 lines (+26/-6)
4 files modified
libunity-2d-private/src/indicatorsmanager.cpp (+10/-2)
libunity-2d-private/src/indicatorsmanager.h (+4/-1)
libunity-2d-private/src/unity2dpanel.cpp (+10/-3)
libunity-2d-private/src/unity2dpanel.h (+2/-0)
To merge this branch: bzr merge lp://qastaging/~3v1n0/unity-2d/fix-multiscreen-indicators-geometries
Reviewer Review Type Date Requested Status
Alberto Mardegan (community) Approve
Review via email: mp+78965@code.qastaging.launchpad.net

Description of the change

Fixing bug #869196 for unity-2d as well, making each panel-window to sync his indicator geometries sending a different panel-id to unity-panel-service.

The code has a "soft" dependency with the latest unity-panel-service. So, these changes can be applied with no problem to unity-2d, but they don't really lead to a fix until you don't run the unity-panel-service from my branch: lp:~3v1n0/unity/fix-multiscreen-indicators-geometries

To post a comment you must log in.
Revision history for this message
Alberto Mardegan (mardy) wrote :

Hi Marco, excellent work! I will test it soon, just looking at the code for now :-)
Meanwhile, a couple of comments:

- Making the Unity2dPanel as implicit parent for the IndicatorsManager can cause confusion. For instance, one could call IndicatorsManager::setParent() and break our assumption. IMHO it's better to have a separate method IndicatorsManager::setPanel(), or a constructor which takes both a Unity2dPanel argument and a parent QObject.

- Member naming: getID() -> id()

- "d->q" = "this" :-)

review: Needs Fixing
756. By Marco Trevisan (Treviño)

Unity2Dpanel and IndicatorsManager: Some code improvements

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

I've fixed what you mentioned. I used the constructor to define the Unity2dPanel in IndicatosManager.

Revision history for this message
Alberto Mardegan (mardy) wrote :

Thanks Marco! Please bear some more patience: could you change this:

IndicatorsManager(QObject* parent, Unity2dPanel* panel);

to

IndicatorsManager(Unity2dPanel* panel, QObject* parent = 0);

The parent argument is always the last, and can be omitted (in our case it's correct to set it though, so you don't have to change the place where it's instantiated).

review: Needs Fixing
757. By Marco Trevisan (Treviño)

IndicatorsManager: invert the initialization parameters.

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Ok, done. I hope it's fine now.

Revision history for this message
Alberto Mardegan (mardy) wrote :

Last nitpicking, I promise :-)
- "Unity2dPanel *panel" -> "Unity2dPanel* panel"
- "QObject* parent = NULL" -> "QObject* parent = 0"

review: Needs Fixing
758. By Marco Trevisan (Treviño)

IndicatorsManager: more code style fixes

Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

I hope that now it will match all your codying style rules... Sorry, but I'm switching too many projects here :)

Revision history for this message
Alberto Mardegan (mardy) wrote :

Fantastic! Thanks Marco!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches