On Sun, May 25, 2008 at 1:30 AM, Kristian Lyngstøl
<[EMAIL PROTECTED]> wrote:
>
> I see several issues here, without even trying to apply the patch....
>
> 1. On several occasions you add commented out code.
I'll remove it in that case
> 2. You are doing more than one thing in one patch. Saving the state
> and animation are two completely unrelated things, and should therefor
> be in two different patches. Your patch is also named "003" which
> suggest you have other patches applied.
Sure thing
> 3. You do stuff like :
> case foo:
> {
> bla
> }
>
> This is not how you use switch statements in C.
I'll use:
case foo:
blah;
break;
then
>
> 4. In several places, you are changing whitespace.
I'll have to look at my text editor for that. I have this feeling that
the indentation
is set incorrectly
> 5. Your option description use traditional useless long descriptions.
> None of my plugins do this. Make a real long description.
I'll try to do this ;-)
> 6. The only sensible comment you have is for a function that's
> commented out, and that's not in the same style used everywhere else
> in the plugin and Compiz in general. We generally use:
>
> /* fooo, bar bla blab alba
> * new line
> * bla bla bla
> */
> or some variant. Like the rest of the comments in maximumize.
Sure ;-)
> 7. + ms->steps = (float) time / ((20.1 - speed) * 10); <--- This
> seems quite random.
I'll probably have a add a comment on how that works. It's kind of like
a geometric series that stops at a particular point, that is, when time is
0. 20.1 - speed is exactly that, the higher the speed, the lower the divisor.
I'm guessing I could refactor it to divide by some constant and multiply by
speed.
> 8. Coding style is generally messy and/or random.
>
Can you give an example so I know what to fix? Thanks =)
_______________________________________________
Dev mailing list
[email protected]
http://lists.compiz-fusion.org/mailman/listinfo/dev
