Code review comment for lp://qastaging/~mehrdada/compiz/compiz.plugins-put

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.