Merge lp://qastaging/~mehrdada/compiz/compiz.plugins-put into lp://qastaging/compiz/0.9.10

+1, Mehrdad Afshari.

Better multimonitor-support is always a great thing :)
Work to improve Compiz' abilities on multiscreen setups is very appreciated !

Code LGTM at first glance, will test this thingy ASAP...

/partially offtopic on
MCR finally has to finally get himself some Display-Port->HDMI connectors, so he can test the 6 output ports of his gfx card ;) -> http://www.tweaktown.com/reviews/3915/asus_radeon_hd_6950_2gb_directcu_ii_overclocked_video_card_review/index2.html
/partially offtopic off

Hi, thanks for this. However, this code looks really confusing and possibly broken.

29 + int outputNum, currentNum;
30 + int nOutputDev = s->outputDevs ().size ();
31 + CompOutput *currentOutput, *newOutput;
32 +
33 + if (nOutputDev < 2)
34 + return result;
35 +
36 + currentNum = getOutputForWindow (w);
37 + outputNum = (currentNum + 1) % nOutputDev;
38 + outputNum = CompOption::getIntOptionNamed (option,"output",
39 + outputNum);
40 +
41 + if (outputNum >= nOutputDev)
42 + return result;
43 +
44 + currentOutput = &s->outputDevs ().at(currentNum);
45 + newOutput = &s->outputDevs ().at(outputNum);
46 +
47 + /* move by the distance of the output center points */
48 + dx = (newOutput->x1 () + newOutput->width () / 2) -
49 + (currentOutput->x1 () + currentOutput->width () / 2);
50 + dy = (newOutput->y1 () + newOutput->height () / 2) -
51 + (currentOutput->y1 () + currentOutput->height () / 2);
52 +
53 + /* update work area for new output */
54 + workArea = newOutput->workArea ();

This looks like an exact copy-paste of PutNextOutput. It relies on no "output" argument being passed to the action callback and then provides (currentNum + 1 % nOutputDev) as the default option, which will be returned as the "previous" output and then the window is just moved to that. So effectively it just moves the window to the next output.

Note that this will work in the two-screen case, since the "previous output" and "next output" are basically the same thing in terms of the modulo operator (eg, 0 + 1 mod 2 is 1 which is the logical "previous output" to 0, and 1 + 1 modulo 2 is 0 which is the logical "previous output" to 1). It won't work in the three-screen case though (1 + 1 modulo 3 is 2 - that is the logical "next output").

So this probably needs fixing in that aspect. I'd suggest two things here:

1. Since the code is basically an exact repeat of the PutNextOutput code, a lot of it can go into a separate function.
2. In the PreviousOutput case, the default argument should be abs ((currentNum - 1) % nOutputDev), noting that a negative number modulo a positive number has an implementation dependent signage.

Yeah, sorry, I already fixed it in the last push I made a few minutes ago. Apparently I made a mistake not merging my changes correctly with the code I pulled from bzr repo and forgetting that line. :)
Anyway, it is fixed. I'll try putting it in a different function though in a minute.

Re the calculation code, I think my current implementation would work fine (always non-negative): (currentNum + nOutput - 1) % nOutput

> Yeah, sorry, I already fixed it in the last push I made a few minutes ago.
> Apparently I made a mistake not merging my changes correctly with the code I
> pulled from bzr repo and forgetting that line. :)
> Anyway, it is fixed. I'll try putting it in a different function though in a
> minute.
>

Thanks :)

> Re the calculation code, I think my current implementation would work fine
> (always non-negative): (currentNum + nOutput - 1) % nOutput

Probably safer to put it behind an abs () based on what the implementation says.

> Probably safer to put it behind an abs () based on what the implementation
> says.

Erm, what the standard says:

The binary / operator yields the quotient, and the binary % operator yields the remainder from the division of the first expression by the second. If the second operand of / or % is zero the behavior is undefined; otherwise (a/b)*b + a%b is equal to a. If both operands are nonnegative then the remainder is nonnegative; if not, the sign of the remainder is implementation-defined.

I don't think abs(...)%nOutputDev is the proper way to handle it. We want "-1" to represent the last display, i.e. display #4 on a 5-monitor system. By abs-ing it, we'll get 1 % 5 = 1, which will move the window to the second display (#1) instead.
(We may want to assert that the expression inside abs is non-negative, but silently taking the absolute value sounds like sweeping something under the rug.)

(Sorry for the mistake again. It happened because I originally made the change on`apt-get source compiz-plugins-main` and then pulled the Bazaar repo and had to merge the changes into it again.)

> I don't think abs(...)%nOutputDev is the proper way to handle it. We want "-1"
> to represent the last display, i.e. display #4 on a 5-monitor system. By abs-
> ing it, we'll get 1 % 5 = 1, which will move the window to the second display
> (#1) instead.

Actually, you're right, usage of abs is probably the wrong way to go here. Testing that the value is negative and then adding the quotient is the right way to handle it:

outputNum = (currentNum - 1) % nOutputDev;
if (outputNum < 0)
    outputNum += nOutputDev;

Thanks for the refactor by the way.

Alright, looks fine to me.

Excellent, thanks!

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~mehrdada/compiz/compiz.plugins-put/+merge/163258/+edit-commit-message

Ah, you'll need to set a commit message.

Speaking of which - can you file a bug and link it to this proposal? You can do that with:

bzr commit --unchanged --fixes lp:bugnumber.

@mehrdada:
Please run 'bzr merge lp:compiz' once again.

The ABI logic has been improved and probably you have to fix a conflict at the bottom of put.cpp - simply use the new version without the negations, save again and 'bzr resolve plugins/src/put.cpp' and 'bzr push' once you are finished.

If 'bzr merge lp:compiz' won't show a text conflict, all is good and you won't have to do anything for this to land...

*it is 'bzr resolve plugins/put/src/put.cpp' of course ;)

> If 'bzr merge lp:compiz' won't show a text conflict, all is good and you won't
> have to do anything for this to land...

It doesn't seem to have any conflicts. Merges cleanly.

>
> It doesn't seem to have any conflicts. Merges cleanly.

Ok, cool - I was not sure what bzr would do, because of the ABI code change ;)

+1. Thanks 4 the extra work :)