Sending again without the attachment (else it gets stuck in ML validation process :/ ).
-------- Original Message -------- Subject: Re: [Bf-committers] uiList enhancements Date: Fri, 23 Aug 2013 15:56:38 +0200 From: Bastien Montagne <[email protected]> To: bf-blender developers <[email protected]> Hi Brecht, So here are "final" new versions of the patches, addressing issues/comments/suggestions from first review: RNA patch: http://www.pasteall.org/45034/diff Optional parameters for py callbacks: http://www.pasteall.org/45035/diff New drag icon: http://www.pasteall.org/44832/diff New dragresize code (with restored maxrows feature): http://www.pasteall.org/45087/diff New UIList filtering code (with enhanced UI): http://www.pasteall.org/45088/diff Note I kept +/- icons to show/hide filtering options for now (imho, it’s also consistent with other parts of Blender, e.g. the show/hide of menus in headers...). Also attached all those guys in archive to this mail! Kind regards, Bastien On 21/08/2013 22:36, Bastien Montagne wrote: > 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 >> <[email protected]> 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 >> [email protected] >> http://lists.blender.org/mailman/listinfo/bf-committers >> > _______________________________________________ > Bf-committers mailing list > [email protected] > http://lists.blender.org/mailman/listinfo/bf-committers _______________________________________________ Bf-committers mailing list [email protected] http://lists.blender.org/mailman/listinfo/bf-committers
