Details

Reviewers

Campbell Barton (campbellbarton)
Sergey Sharybin (sergey)

Commits

rBS9e2abbc9ba5d: FileBrowser: Editable Bookmarks.
rB9e2abbc9ba5d: FileBrowser: Editable Bookmarks.

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
Branch: bookmarks-temp

Event Timeline

Bastien Montagne (mont29) updated this revision to Diff 3448.Feb 10 2015, 3:09 PM

Bastien Montagne (mont29) retitled this revision from to From asset-browser branch: UILists for bookmarks, with moving and renaming..

Bastien Montagne (mont29) updated this object.

Bastien Montagne (mont29) added reviewers: Campbell Barton (campbellbarton), Sergey Sharybin (sergey).

Bastien Montagne (mont29) added a subscriber: Manu Järvinen (maxon).

Sergey Sharybin (sergey) requested changes to this revision.Feb 10 2015, 3:37 PM

Sergey Sharybin (sergey) edited edge metadata.

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
616–667	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);

Bastien Montagne (mont29) added inline comments.Feb 10 2015, 3:58 PM

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) edited edge metadata.

Campbell Barton (campbellbarton) added inline comments.

source/blender/editors/interface/interface_intern.h
257–261	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	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

Campbell Barton (campbellbarton) added inline comments.Feb 10 2015, 5:46 PM

source/blender/editors/interface/interface_intern.h
257–261	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.

Greg Zaal (gregzaal) added a subscriber: Greg Zaal (gregzaal).Feb 10 2015, 5:59 PM

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.

Campbell Barton (campbellbarton) added inline comments.Feb 10 2015, 10:16 PM

source/blender/editors/interface/interface.c
4123	(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
538	may as well call `RNA_property_int_get` since you have the prop.

Bastien Montagne (mont29) added inline comments.Feb 10 2015, 10:28 PM

source/blender/editors/interface/interface.c
4123	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.

Minor fixes.

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	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.

Campbell Barton (campbellbarton) accepted this revision.Feb 10 2015, 11:40 PM

Campbell Barton (campbellbarton) edited edge metadata.

Closed by commit rB9e2abbc9ba5d: FileBrowser: Editable Bookmarks. (authored by Bastien Montagne (mont29)). · Explain WhyFeb 11 2015, 12:22 AM

This revision was automatically updated to reflect the committed changes.

Pablo Vazquez (pablovazquez) awarded a token.Feb 11 2015, 12:41 AM

Manu Järvinen (maxon) awarded a token.Feb 11 2015, 12:34 PM

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 3464

release/scripts/startup/bl_ui/space_filebrowser.py

source/blender/blenloader/intern/versioning_270.c

source/blender/editors/include/ED_fileselect.h

source/blender/editors/include/UI_interface.h

source/blender/editors/interface/interface.c

source/blender/editors/interface/interface_intern.h

source/blender/editors/interface/interface_templates.c

source/blender/editors/screen/area.c

source/blender/editors/space_file/file_draw.c

source/blender/editors/space_file/file_intern.h

source/blender/editors/space_file/file_ops.c

source/blender/editors/space_file/file_panels.c

source/blender/editors/space_file/filesel.c

source/blender/editors/space_file/fsmenu.h

source/blender/editors/space_file/fsmenu.c

source/blender/editors/space_file/space_file.c

source/blender/editors/space_node/drawnode.c

source/blender/editors/space_node/node_buttons.c

source/blender/makesdna/DNA_space_types.h

source/blender/makesrna/RNA_access.h

source/blender/makesrna/intern/rna_space.c

source/blender/makesrna/intern/rna_ui_api.c

source/blenderplayer/bad_level_call_stubs/stubs.c

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 3464

release/scripts/startup/bl_ui/space_filebrowser.py

source/blender/blenloader/intern/versioning_270.c

source/blender/editors/include/ED_fileselect.h

source/blender/editors/include/UI_interface.h

source/blender/editors/interface/interface.c

source/blender/editors/interface/interface_intern.h

source/blender/editors/interface/interface_templates.c

source/blender/editors/screen/area.c

source/blender/editors/space_file/file_draw.c

source/blender/editors/space_file/file_intern.h

source/blender/editors/space_file/file_ops.c

source/blender/editors/space_file/file_panels.c

source/blender/editors/space_file/filesel.c

source/blender/editors/space_file/fsmenu.h

source/blender/editors/space_file/fsmenu.c

source/blender/editors/space_file/space_file.c

source/blender/editors/space_node/drawnode.c

source/blender/editors/space_node/node_buttons.c

source/blender/makesdna/DNA_space_types.h

source/blender/makesrna/RNA_access.h

source/blender/makesrna/intern/rna_space.c

source/blender/makesrna/intern/rna_ui_api.c

source/blenderplayer/bad_level_call_stubs/stubs.c

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

Revision Contents
Changeset List