Merge lp://staging/~gang65/unity-2d/new-app-menu into lp://staging/unity-2d

Proposed by Bartosz Kosiorek
Status: Needs review
Proposed branch: lp://staging/~gang65/unity-2d/new-app-menu
Merge into: lp://staging/unity-2d
Diff against target: 218 lines (+150/-3)
4 files modified
panel/applets/appname/appnameapplet.cpp (+1/-0)
panel/applets/appname/windowhelper.cpp (+29/-3)
panel/applets/appname/windowhelper.h (+5/-0)
tests/panel/new_app_menu_display.rb (+115/-0)
To merge this branch: bzr merge lp://staging/~gang65/unity-2d/new-app-menu
Reviewer Review Type Date Requested Status
Gerry Boland (community) Needs Fixing
PS Jenkins bot continuous-integration Pending
Review via email: mp+95238@code.staging.launchpad.net

Description of the change

It it my first unity-2d hack, so please check this carefully :-)

To post a comment you must log in.
Revision history for this message
Omer Akram (om26er) wrote :

Diff against target: 0 lines :-P

Revision history for this message
Bartosz Kosiorek (gang65) wrote :

Sorry.

Already fixed.

Revision history for this message
Gerry Boland (gerboland) wrote :

Hey Bartosz,
thanks for this, I've some comments on how we can make it better.

First off, when testing functionally, I noticed that every new window opening triggered the menu to show. We want this to be more specific: to only show the menu for the first window of a newly opened application. So opening Terminal for the first time, the menu should appear for a little. But if you open another Terminal, no menu will appear.

So the difficulty is figuring out when a new application opens a window. This can be solved if you take a look at the comment in libunity-2d-private/src/windowlist.cpp, line 139: The application/window matcher library BAMF, sends 2 ViewOpened signals if a new application opens a window. This should be useful.

Ultimately I would like to see is WindowHelper having a signal/property like "newApplicationWindow" - that's all we really care about.

Then for me it doesn't make sense for the WindowHelper - whose purpose is more to get info from & control a window - to deal with the logic for the application menu. This IMO belongs to AppnameApplet.

Let me know what you think about my ideas. We can chat more via email if you wish.

I'm delighted to see you've written a test. Testing is important for us! Yep it's a bit blank right now, but I'd be happy to help you with it :)

Thanks for your excellent work so far.
-Gerry

review: Needs Fixing

Unmerged revisions

918. By Bartosz Kosiorek

Add initial test cases

917. By Bartosz Kosiorek

Add new application's menu (#874254)

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