[vlc-devel] [PATCH] Puzzle filter enhancement

[Vianney BOYER]

PART 1/3 : comments



Thanks for your time spent to look at my code.
You may have supposed that my first objective was to obtain a working executable without any warning when compiling it on my computer! ...and without any segmentation fault...
Anyway, I understand your remarks as you want a code that anyone will be able to understand the code and change it when needed.
I'll do all the modification you've requested as, myself, I was happy to understand and find in the other files of this project some help for specific issues.
Please be indulgent as this is my first contribution on such project and I'm not aware of such "best practice".
...but don't hesitate to ask for other modifications... and I hope that I've well understood your comments...

    >The patch got badly mutilated by your mail agent, and consequently they
    >cannot be applied due to format corruption. With Microsoft Outlook, your
    >only option is to attach the patches as files instead.
now I've sent it with msmtp.
I hope this will avoid corruption
    
    >With that being said, the patch is so large that it chokes my crappy mail
    >editor. Clearly, my mail editor is at fault for being so poorly optimized.
    >But 3000 lines of code is quite much for a single reviewer at once, at
    >least it's too much for me. I would strongly recommend splitting the patch
    >logically into a changes set of multiple consecutive and incremental
    >patches.
incremental patches are not suitable as whole code has been changed
However, now I've sent 2 different patches 1 for GUI and 1 for code. Hope it will help


    >> Original puzzle filter is still available. You just have to disable
    >> "advanced mode" parameter.
    >Why ? I see no reason for this.
In fact this is not original code as I have fixed some bugs (pictures' planes...) and it was not compatible with my code.
but I kept it because:
1/ Both puzzle style shapes and drad'n drop increases CPU usage and may be a limitation on some systems. Basic mode may be more fluent on slow computers.
2/ Sometimes I use this original/basic puzzle mode. It's just different: a sliding puzzle instead of a jigsaw game.


    >Please split UI from puzzle.c modifications.
done (just copy/paste the path I hope it will works)
take care, you have to install both patch at the same time
otherwise it will not work

    >Do not change the mode of the files, please.
sorry

    >use _MAX and _MIN
done

    >> +#define init_countdown(init_val) ( (MAX( 1, 30000 - init_val)/20) / 2 + (
    >> (unsigned) vlc_mrand48() ) % ( MAX( 1, ((30000 - init_val)/20) ) ) )
    >Static inline it.
done

    >> +struct filter_sys_t {
    >> +    bool b_init;
    >> +    bool b_bake_request;
    >> +    bool b_shape_init;
    >> +    param_t s_allocated;
    >> +    param_t s_current_param;
    >> +    param_t s_new_param;
    >No param_t* ?
Why?

    >You should reorder for better repacking.
done
    >You should align comments a bit better.
done

    >callocing would make the code shorter.
done

    >> +    p_sys->i_preview_pos = 0;// 0=top-left, top-right, bottom right,
    >> bottom-left
    >Is this comment at the right place?
yes but only clear for me... changed!

    >> +    p_sys->ps_bezier_pts_H = malloc( sizeof( point_t *) * SHAPES_QTY );
    >Check your malloc. You should check this on all your code.
You'll kill me but where is the mistake? ps_bezier_pts_H is of type point_t **

    >free(NULL) is a no-op. You should check this on all your code.
changed

    >> +            p_sys->s_current_param.i_auto_solve_speed    = 0;
    >> +            p_sys->s_current_param.i_rotate     = 0;
    >memset it?
no, only few members are set

    >> +                    int32_t i_group_ID = p_sys->ps_pieces[0].i_group_ID;
    >Why not a uint32_t btw?
changed. (Why? because at the begining I was using -1 for stand alone pieces...)

    >Use ternary
done

    >> +    p_sys->s_allocated.i_shape_size = p_sys->s_current_param.i_shape_size;
    >> +    p_sys->s_allocated.b_advanced = p_sys->s_current_param.b_advanced;
    >> +    p_sys->s_allocated.i_auto_shuffle_speed =
    >> p_sys->s_current_param.i_auto_shuffle_speed;
    >> +    p_sys->s_allocated.i_auto_solve_speed =
    >> p_sys->s_current_param.i_auto_solve_speed;
    >> +    p_sys->s_allocated.i_rotate = p_sys->s_current_param.i_rotate;

    >Vertical aligning could help a bit the readability.
done

    >> +    return 0;
    >VLC_SUCCESS maybe?
changed

    >> +    int32_t i;
    >> +
    >> +    i = 0;
    >You can combine both lines :)
This one I was able to find it myself...

    >On functions like this, you should invert your first if and return
    >directly there, reduce one level of indentation and remove the last
    >useless return.
done

    >> +    memset ( p_sys->pi_group_qty, (int32_t) 0, sizeof( int32_t ) *
    >> (p_sys->s_allocated.i_pieces_nbr) );
    >Are you sure the cast is necessary?
changed

    >> +                                                    {
    >> +                                                        if
    >> (p_sys->i_mouse_y == i_current_dst_y )
    >Please reduce indentation levels...
    >13 is a bit too much...
15 to be exact!
I've reduced it to 13 by merging 3 if statements.

    >> +            f_bez_x = ( 1 - f_sub_t ) * ( 1 - f_sub_t ) * ( 1 - f_sub_t ) *
    >> ps_pt[ 3 * i_main_t ].f_x
    >> +                    + 3 * f_sub_t * ( 1 - f_sub_t ) * ( 1 - f_sub_t ) *
    >> ps_pt[ 3 * i_main_t + 1 ].f_x
    >> +                    + 3 * f_sub_t * f_sub_t * ( 1 - f_sub_t ) * ps_pt[ 3 *
    >> i_main_t + 2 ].f_x
    >> +                    + f_sub_t * f_sub_t * f_sub_t * ps_pt[ 3 * i_main_t + 3
    >> ].f_x;
    >Is it possible to macro those?
done

    >> +    ofp = fopen(sav_filename, "w");
    >No. Use VLC functions.
removed: memory used instead of file.
At the begining I wanted to implement a load/save function. But I was unable to modify GUI to set parameters as per loaded data.
I couldn't find any example in the other filters. How can I do that?

I tried to apply your comment everywhere feel free to ask for more...

    >In general, you should increase a bit the readability of your code
sure
    >and check for common errors.
I have not found any error at present... but I have to make it more clear!