From asset-browser branch: UILists for bookmarks, with moving and renaming.
ClosedPublic

Authored by Bastien Montagne (mont29) on Feb 10 2015, 3:09 PM.

Details

Summary

Quite a bit of code in the end - mostly boring boiling one though.

Note this also introduce the 'migration' from a single 'CHANNEL' area in file browser,
to a 'TOOLS' one, with dedicated 'TOOL PROPS' one for operator options, like in 3DView.
This is in preparation for more complex needs of real asset stuff, where tabs will be
definitively needed - and also because since operator panel remains defined in C, it
would otherwise always be on top, which is not good!

Had to do some tweaks to a few side areas too, like e.g. enabling a more advanced
behavior of tooltips, to allow keeping display of the full path of the bookmarks there.

Diff Detail

Repository
rB Blender
Bastien Montagne (mont29) retitled this revision from to From asset-browser branch: UILists for bookmarks, with moving and renaming..Feb 10 2015, 3:09 PM
Bastien Montagne (mont29) updated this object.
Sergey Sharybin (sergey) requested changes to this revision.Feb 10 2015, 3:37 PM
Sergey Sharybin (sergey) added inline comments.
source/blender/blenloader/intern/versioning_270.c
597

You can't just interrupt the versioning code..

If it's a bug then put BLI_assert() here and expect ar to always be != NULL.

600

Usually this is done via things like ED_clip_has_channels_region(), making the space to worry about all the initialization.

This revision now requires changes to proceed.Feb 10 2015, 3:37 PM

Looked over, seems quite good.

source/blender/editors/include/ED_fileselect.h
123–126

naming is a bit mixed FS_INSERT_APPEND_LAST ?

source/blender/editors/space_file/file_ops.c
618–669

Would try to split out listbase logic if possible, seems fairly generic? - but if not... no worries.

getting this error building.

/src/blender/source/blender/editors/interface/interface_layout.c:721:13: error: assignment discards 'const' qualifier from pointer target type [-Werror]

but->tip = item->description;
         ^

/src/blender/source/blender/editors/interface/interface_layout.c: In function 'uiLayoutListBox':
/src/blender/source/blender/editors/interface/interface_layout.c:2749:12: error: assignment discards 'const' qualifier from pointer target type [-Werror]

but->tip = RNA_property_description(actprop);
source/blender/blenloader/intern/versioning_270.c
600

I tried using file_tools_region(sa) from space_file.c's file_refresh(), but does not really work as I'd expect (have to toggle T that area to show it…

And we'd also loose the 'hidden' status.

  • Various fixes and enhacements from first review round (remove new regions handling from versioning code, fix issues with but->tip direct assignements in layout code).
  • Factorized core logic of bookmark_move_exec into new BLI_linklist_move_item.
Campbell Barton (campbellbarton) requested changes to this revision.Feb 10 2015, 5:43 PM
Campbell Barton (campbellbarton) added inline comments.
source/blender/editors/interface/interface_intern.h
257 ↗(On Diff #3454)

Would rather not have tooltip data in every button. just because in very rare cases we might want an allocated tip.
this is really an exceptional case.

Why not just have a flag on the button to note that the tip is malloc'd, so it gets freed along with the button?

258 ↗(On Diff #3454)

Would rather keep this a const char * - MEM_freeN can just cast to void, if this happens to be allocated.

This revision now requires changes to proceed.Feb 10 2015, 5:43 PM
source/blender/editors/interface/interface_intern.h
257 ↗(On Diff #3454)

Another option could be to have a tip callback, its a bit silly to allocate the tip on every draw - an optional callback could run when the tip is needed.

Trying the callback for advanced (dynamic/generated) tooltips.

Works quite nice, but not sure to be really hapy with the need of another PointerRNA/PropertyRNA
here? Could not think of another way to avoid allocated arg in our usecase though.

Also, made handling of this new option in UIList template a bit nicer, now you can
use any item property as 'custom' tooltip.

  • Rework uiBut dyntooltip to use a callback.

use tip_argN (ie. freed arg) for dyntips.

source/blender/editors/interface/interface.c
4123 ↗(On Diff #3462)

(paranoid) - would add assert here... BLI_assert(func == NULL || but->tip == NULL); if both are valid... its a bit strange?

source/blender/editors/space_file/file_ops.c
539

may as well call RNA_property_int_get since you have the prop.

source/blender/editors/interface/interface.c
4123 ↗(On Diff #3462)

No, but->tip is passed to func too, can be an easy way to store static, invariant piece of the tooltip (like we do for uilist items). So both may be valid at the same time.

Campbell Barton (campbellbarton) requested changes to this revision.Feb 10 2015, 10:41 PM

Renaming a bookmark, then immediately isnt saving the rename, Im not sure how this is working exactly since I managed to get it to rename the bookmark once.

source/blender/editors/interface/interface.c
4123 ↗(On Diff #3462)

In that case a brief comment in button struct would be helpful.

This revision now requires changes to proceed.Feb 10 2015, 10:41 PM

Fix bookmarks renaming not stored in most cases...

Added write to file in ED_fsmenu_entry_set_name/_path(), not ideal, but should be OK in this case for now.

Ideally, this should be handled by some kind of 'dirty' flag, this needs cleanup anyway, but out of scope of current patch.

Noticed very minor glitch, adding a new bookmark doesn't activate it (go to edit text, but dont change any text of the path, press enter - makes the newly added bookmark active)

  • Fix missing 'refresh' of active bookmarks in add/delete ops.
This revision was automatically updated to reflect the committed changes.