On 04/24/2011 04:10 PM, Tomas Gavenciak wrote:
Hi all,
First question first: Budeme psat do listu jen anglicky, nebo je cestina OK?
Ahoj, ahoj,
Jsem pro anglictinu. Vsichni rozumime, vypada to vic l33t a navic, kdyby chtel nekdo ze zahranici opravit chybu, poslat patch nebo dokonce spolupracovat (clovek nikdy nevi), bude velmi vyhodne, kdyz si bude moci precist archiv konference.
Jo: prosim pana teto konference, aby nastavil 'Reply To' u mailu na gfxprim@ucw.cz; lepe se pak odpovida.
Before going to the individual ideas, I would like to ask you for your opinions and for help with refactoring the current code (including names), thinking about the people that will use the library, frequent use cases and "most-expected" behaviour.
I would like to propose few renames in GfxPrim, mostly to improve readability:
GP_Context: x_swap, y_swap -> flip_x, flip_y It is called "vertical/horizontal flip", meaningful-sentence-like identifiers are easier to remember and read (flip_x vs x_flip) GP_Context: axes_swap -> swap_axes Readability, consistency with flip_* GP_Context: clip_(w|h)_(min|max) -> clip_x_min etc. clipping coordinate, not width
All three are OK with me; just a note: I personally use 'xmin' and 'xmax' (slightly shorter) but this is not much an issue.
The argument-modifying macros do not indicate which arguments are modified (even worse than C++ refs).
True, and I accept that it's ugly; we did not have any better idea.
I propose to at least consistently document this in macro defs and/or change the argument names from "x" to "x_mod" or similar (i.e. in GP_Transform.h". Is there a LKML convention for that?
I think this is not a bad idea, but '_mod' is IMO misleading (can mean anything else, e.g. 'modulus'). Maybe 'x_out'?
While we are at it, we might unify GP_IS_CONTEXT_VALID and GP_CHECK_CONTEXT. Is the distinction intentional?
GP_IS_CONTEXT_VALID is just a test, GP_CHECK_CONTEXT is an assertion. Both are ugly and probably should not exist as they do now; when I wrote them, I tried to do as many safety checks as possible, like in Linux kernel, but I probably picked the wrong way to do it.
It definitely makes sense to have transforming+clipping versions of operations as well as "bare" unsafe variants. I would propose to remove the "T" prefix from transforming variants, as most users will want the transforming behavior and to modify the names of the "bare" functions. I would propose "GP_Line_" or "GP_Line_Bare". What do you think?
I agree with the transformed functions being the default. However, "_Bare" is uncommon and IMO difficult to understand. I'm not even sure if the untransformed functions need to be public. The time cost of transformation is only an issue for very basic functions like putpixel; even for lines it should already be negligible.
Next proposal/question is about declaring public vs. private APIs. Most of it is public by default, but then we have accessors for GP_Context and others. What is the goal there?
Do we want GP_Context to be (partially) public, or do we want to provide accessors for everything? I would propose to make it public (to simplify things) and to solve transformation problems by having different fields for "user" w/h and "real" w/h.
I would disagree here; two fields would, IMO, lead to confusion.
We need to somehow fix the context behavior anyway, and the freedom we will retain by using accessors will be probably very small.
Depends on how much 'intelligence' we want to put into the context later. When I think about the API, I feel more and more inclined to adding more status fields - like current color, etc. - to the Context to keep the number of arguments to drawing functions relatively small; otherwise, we will need to pass more and more various style-related settings through arguments. But that's just my opinion.
Best regards,
Jiri Dluhos