Code review comment for lp://staging/~gang65/unity-2d/new-app-menu

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

« Back to merge proposal