[Bf-committers] uiList enhancements

[Bastien Montagne]

Hi Brecht,

Thanks for this first review, and sorry for late reply, but when I 
re-applied the patches here I got a nasty segfault... Turned out to be a 
stupid mistake in rna_UIList_filter_arrays_get_length(), which actually 
got a FunctionRNA as PointerRNA, not the object (i.e. expected 
PointerRNA of uiList here) - anyway, I did not need this func at all 
(dynamic props are really confusing, esp. when used as func parameters 
:/ ). Why this worked smooth a week ago will remain black magic to me, 
esp. when using gcc's sanitizer...

So, here is an updated RNA-fixes patch, taking into account your remarks 
as well as making new fixes for issues I found this evening: 
http://www.pasteall.org/45034/diff

Also, here is a patch adding support for "optional" parameters in Python 
callbacks (the idea is that all params after the first one using 
PROP_PYFUNC_OPTIONAL flag are considered optional, and only passed to py 
func if it actually "has room" for them in its parameter list). This 
should allow much smoother evolution when making non-crucial changes in 
the RNA API: http://www.pasteall.org/45035/diff

(Pacthes adding dragsize and dragicon remain the same for now: 
http://www.pasteall.org/44831/diff and http://www.pasteall.org/44832/diff ).

And the corrected patch adding filtering to uiLists: 
http://www.pasteall.org/45036/diff

And now the answers:

On 21/08/2013 17:14, Brecht Van Lommel wrote:
> Quick UI review, still need to dig into patches:
>
> * The vertex groups panel seems to have two lists, doesn't make sense
> to me, was that for testing?
Yes (actually there are the three layouts, DEFAULT, GRID and COMPACT), 
will obviously be removed before commit. ;)

> * Adding a vertex group gives a memory leak "Error: Not freed memory
> blocks: 2" "rna_ui.c:371 len: 4 0x7fc990c102c8"
Do not have this, but may be related to the bug I fixed in new patch?

> * The filter options don't look very polished to me. Some suggestions
> ** Replace (+) icon by a small magnifying glass? I also would expect
> in the right-bottom.
Yes, why not… but I'll have to add yet another icon.

> ** Filter by Name is cut off, I think if there's a magnifying glass
> that label can be left out.
> ** Put everything on a single line, make sorting options into an enum.
> ** I'm not sure if the invert options are needed, for sorting seems
> unnecessary, and for filter advantage is also unclear to me.
> ** For vertex groups I don't think Filter by Empty and Filter by Name
> should be mutually exclusive, think it could always show textbox and
> have option to show empty.
All this is actually defined in py code, so rather easy to tweak (in the 
limits of UI API). I would keep invert options though (but perhaps as 
toggle icons too, would take much less room), sometimes it's much easier 
to say "I don't want this" rather than "I want that and that and...", 
the same goes with filtering, imho.

> * There is too much padding between last item and gripper, like the
> gripper is a full row but doesn't need to be?
Yes, I noted that too, still don't know why, I would expect it to be the 
height I give the two buttons here, UI_UNIT_Y * 0.4... Looks like a row 
layout from a box one is always at least UI_UNIT_Y height?

As for maxrows, indeed I overlooked materials and textures (at least). 
:/ Will add this back asap.

> On Thu, Aug 15, 2013 at 12:43 AM, Bastien Montagne
> <montagne29 at wanadoo.fr>  wrote:
>> So, first one fixes some bugs with PROP_DYNAMIC RNA parameters (for RNA
>> functions) - unless I completely misunderstood that piece of code, that
>> kind of props was not handled nicely!
>>
>>       http://www.pasteall.org/44830/diff
>> int size = 0
> Change this to size_t.
>
>> /* Only for PROP_DYNAMIC properties! */
> Can you rename the functions to contain the word 'dynamic' to indicate this?
>
>> * I decided to drop the "maxrows" option of list template, afaict it's
>> not used anywhere, and it added quite some unneeded complication to
>> template code (especially as we now support the same kind of features
>> for GRID layout as well).
> It's definitely used, it automatically makes lists expand a few rows
> when you add items. Keeps the lists compact when empty but not
> unreasonably small when it has items. That doesn't seem to be working
> anymore, I thought this was a nice feature.
>
>> I put all the "API maintenance" edits to scripts in a separate patch:
>>
>>       http://www.pasteall.org/44835/diff
> As discussed on IRC, the extra draw_item parameter breaks
> compatibility, would be nice if that was made optional somehow.
> _______________________________________________
> Bf-committers mailing list
> Bf-committers at blender.org
> http://lists.blender.org/mailman/listinfo/bf-committers
>