Merge lp://qastaging/~dyams/unity-2d/update-pips into lp://qastaging/unity-2d

You have some Multi-monitor specific code in this that isn't really quite necessary just now. Just target this MR to indicating if applications are on the current workspace (i.e. visible on all monitors) or not.

+ objectName: "pips-" + item.name+index

Why do you need this? Won't your test work with

@app.Unity2dPanel() \
.LauncherList( :name => 'main' ) \
.QDeclarativeItem( :name => 'Calculator' ) \
.QDeclarativeImage( :name => 'pips')['source']

> + objectName: "pips-" + item.name+index
>
> Why do you need this? Won't your test work with
>
> @app.Unity2dPanel() \
> .LauncherList( :name => 'main' ) \
> .QDeclarativeItem( :name => 'Calculator' ) \
> .QDeclarativeImage( :name => 'pips')['source']
No. There can be more than one pip for the same tile. Testability will not identify the objects if they don't have unique object names.

Aha, the pips are drawn by a repeater, so the original objectName isn't good. Do change it, but still no need for the item.name IMO.

About your test suite: this makes a big change so it needs several tests.

First off can you please add a good test-description as a comment to the top of each test-case, like
https://bazaar.launchpad.net/~unity-2d-team/unity-2d/trunk/view/head:/tests/launcher/autohide_show_tests.rb

I'd also prefer you split the test cases just a little, so you get tests for:
1. One app on this workspace only
2. One app on other workspace only
3. Two apps on this workspace
4. Two apps, both on other workspace
5. Two apps, one on this workspace, one on other
6. Three apps all on this workspace
7. Three apps, all but one on this workspace
I know it's a lot, but these are all the possible cases that I see your code influencing.

This code isn't perfect:
+ Timeout.timeout(3){XDo::XWindow.wait_for_window('Calculator')}
When you've one calculator open, this will return immediately. It is possible then that
+ return XDo::XWindow.from_active
will return the first calculator XId.

This is the reason I was using terminals with unique titlebars in other tests. At least then you can uniquely identify the window. Hacky, I know. If you know of a way to run a program & get the XId of the window reliably, I'd love to hear it! (via PID is a good idea, except for Terminal:) )

Have a look at tests/misc/lib/tmpwindow.rb (in latest trunk). It also has a cleanup method so that if a test fails, you don't have non-closed windows to worry about. It will mean you can remove "kill" statements from the start of test-cases. Call the cleanup method in the teardown, it's

@number_of_workspaces - this can be a local variable, so no @.

+ width = %x{xrandr --current | grep \'\*+\' | uniq | awk \'{print $1}\' | cut -d \'x\' -f1}
It would be great if you added this to the XDoTool Ruby bindings for everyone to use. Parsing the output with Ruby would probably be quicker?? :)

Small "puts" comments in your test suite are not necessary either. And there's some extra blank lines.

Minor things:

if (!m_application)
   return windowCount;
Doesn't follow our coding guidelines.

QRect desktopRect = ScreenInfo::instance()->geometry() ; <- rogue space character

if (totalWindows && !windowsInCurrentWorkspace) { <- both variables are integers, is clearer to be a liite more verbose: (totalWindows > 0) etc..

Question: Why LauncherItem::belongsToDifferentWorkspace() always returns false?

Functionally the code works well and is looking good!

> About your test suite: this makes a big change so it needs several tests.
>
> First off can you please add a good test-description as a comment to the top
> of each test-case, like
> https://bazaar.launchpad.net/~unity-2d-team/unity-
> 2d/trunk/view/head:/tests/launcher/autohide_show_tests.rb
>
Done

> I'd also prefer you split the test cases just a little, so you get tests for:
> 1. One app on this workspace only
> 2. One app on other workspace only
> 3. Two apps on this workspace
> 4. Two apps, both on other workspace
> 5. Two apps, one on this workspace, one on other
> 6. Three apps all on this workspace
> 7. Three apps, all but one on this workspace
> I know it's a lot, but these are all the possible cases that I see your code
> influencing.
Actually current test suite considers them all. By Two apps you mean an app with two windows not two different applications. no?
Well, current tests already have separate tests for app with only one window and app with more than one window.
When testing with more than one window, i have also tested by moving each window to different workspace individually.
1) Two windows on current workspace
2) One window on current workspace and another window on different workspace
3) Both windows on different workspace than current

>
>
> This code isn't perfect:
> + Timeout.timeout(3){XDo::XWindow.wait_for_window('Calculator')}
> When you've one calculator open, this will return immediately. It is possible
> then that
> + return XDo::XWindow.from_active
> will return the first calculator XId.
>
There is a 'sleep 2' in between the two statements, which helped me to address the issue you have mentioned.

> This is the reason I was using terminals with unique titlebars in other tests.
> At least then you can uniquely identify the window. Hacky, I know. If you know
> of a way to run a program & get the XId of the window reliably, I'd love to
> hear it! (via PID is a good idea, except for Terminal:) )
I can borrow the same idea. If you don't like the 'sleep 2' :)

>
> Have a look at tests/misc/lib/tmpwindow.rb (in latest trunk). It also has a
> cleanup method so that if a test fails, you don't have non-closed windows to
> worry about. It will mean you can remove "kill" statements from the start of
> test-cases. Call the cleanup method in the teardown, it's
>
No, this kill statements are not for cleaning the residues of the test case. Instead it is a prerequisite. If there are more than one window then pip image is different than when there is only one window. So we need to ensure there is no running windows of the test application.

>
> @number_of_workspaces - this can be a local variable, so no @.
>
If I remove that @ I got this error
.update_pips_tests.rb:62: undefined local variable or method `number_of_workspaces' for #<Class:0xb6e7cbf4> (NameError)

> + width = %x{xrandr --current | grep \'\*+\' | uniq | awk \'{print $1}\' | cut
> -d \'x\' -f1}
> It would be great if you added this to the XDoTool Ruby bindings for everyone
> to use. Parsing the output with Ruby would probably be quicker?? :)
>
This is part of multi-monitor stuff, which is totally removed from this MR already
>
> Small "puts" comment...

> Minor things:
>
> if (!m_application)
> return windowCount;
> Doesn't follow our coding guidelines.
>
>
> QRect desktopRect = ScreenInfo::instance()->geometry() ; <- rogue space
> character
>
>
> if (totalWindows && !windowsInCurrentWorkspace) { <- both variables are
> integers, is clearer to be a liite more verbose: (totalWindows > 0) etc..
>
>

Done

>
> Question: Why LauncherItem::belongsToDifferentWorkspace() always returns
> false?

It is virtual funtion, So LauncherApplication::belongsToDifferentWorkspace() is what actually gets called. I don't remember why i returned 'false' from there though.

> > Small "puts" comments in your test suite are not necessary either. And
> there's
> > some extra blank lines.
> I have removed 'puts'. I thought they would inform what the test case is doing
> while we are waiting for them to finish. Unfortunately they did serve the
> purpose it seems;)>
I have removed 'puts'. I added them with the intention of informing the nature of tests being executed.
Unfortunately they didn't serve the purpose, it seems :)

I recommend you make one commit for every comment considered. It makes reviewing your changes easier.

> Actually current test suite considers them all. By Two apps you mean an app
> with two windows not two different applications. no?
> Well, current tests already have separate tests for app with only one window
> and app with more than one window.
> When testing with more than one window, i have also tested by moving each
> window to different workspace individually.
> 1) Two windows on current workspace
> 2) One window on current workspace and another window on different workspace
> 3) Both windows on different workspace than current

Indeed I noticed that. I just think that each of your test cases are doing too much in one go. Hence I wanted the split.

> > This code isn't perfect:
> > + Timeout.timeout(3){XDo::XWindow.wait_for_window('Calculator')}
> > When you've one calculator open, this will return immediately. It is
> possible
> > then that
> > + return XDo::XWindow.from_active
> > will return the first calculator XId.
> >
> There is a 'sleep 2' in between the two statements, which helped me to address
> the issue you have mentioned.

Helps, but not a cure. We cannot assume the machine isn't slow, we don't want failures because of it.

> I can borrow the same idea. If you don't like the 'sleep 2' :)

Please use the tmpwindow lib. It's a bit safer.

>
> >
> > Have a look at tests/misc/lib/tmpwindow.rb (in latest trunk). It also has a
> > cleanup method so that if a test fails, you don't have non-closed windows to
> > worry about. It will mean you can remove "kill" statements from the start of
> > test-cases. Call the cleanup method in the teardown, it's
> >
> No, this kill statements are not for cleaning the residues of the test case.
> Instead it is a prerequisite. If there are more than one window then pip image
> is different than when there is only one window. So we need to ensure there is
> no running windows of the test application.

Ok, in which case, it should be part of "setup" - or maybe "startup" and then being sure that "teardown" closes all windows created during tests.

> > @number_of_workspaces - this can be a local variable, so no @.
> >
> If I remove that @ I got this error
> .update_pips_tests.rb:62: undefined local variable or method
> `number_of_workspaces' for #<Class:0xb6e7cbf4> (NameError)

Oh you use it in the shutdown, I missed that. Apologies.

> > + width = %x{xrandr --current | grep \'\*+\' | uniq | awk \'{print $1}\' |
> cut
> > -d \'x\' -f1}
> > It would be great if you added this to the XDoTool Ruby bindings for
> everyone
> > to use. Parsing the output with Ruby would probably be quicker?? :)
> >
> This is part of multi-monitor stuff, which is totally removed from this MR
> already

Ok.

> I have removed 'puts'. I thought they would inform what the test case is doing
> while we are waiting for them to finish. Unfortunately they did serve the
> purpose it seems;)

I don't care what test is happening, until it fails, in which case the error tells you what test failed. These comments are nice, but will litter log files, so I prefer to not have them.

Since we're reviewing a MR for this exact feature to go into shell, this MR is obselete, no?

This has been merged into shell, so I consider this branch obselete and thus rejecting.

Works well in shell tho, nice work.