On Sat, May 24, 2008 at 6:13 PM, Sam Spilsbury <[EMAIL PROTECTED]> wrote:
> Attatched is a patch to add animation to the maximumization of a window.
> The patch also adds structs, init functions and other things in order to
> make this possible. It also adds the ability to 'toggle' between a
> maximumized state and a non-maximumized state
I see several issues here, without even trying to apply the patch....
1. On several occasions you add commented out code.
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.
3. You do stuff like :
case foo:
{
bla
}
This is not how you use switch statements in C.
4. In several places, you are changing whitespace.
5. Your option description use traditional useless long descriptions.
None of my plugins do this. Make a real long description.
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.
7. + ms->steps = (float) time / ((20.1 - speed) * 10); <--- This
seems quite random.
8. Coding style is generally messy and/or random.
Seriously Sam.... Read your patch before you send it to others.
I am not going to comment on functionality, because you should cleanup
the above points, then re-post it, and I will look at it more closely.
- Kristian
_______________________________________________
Dev mailing list
[email protected]
http://lists.compiz-fusion.org/mailman/listinfo/dev
