Page MenuHome
Differential D1093

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

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

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
Campbell Barton (campbellbarton) edited edge metadata.Feb 10 2015, 3:46 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.

Campbell Barton (campbellbarton) added a comment.Feb 10 2015, 3:57 PM

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.

Bastien Montagne (mont29) updated this revision to Diff 3454.Feb 10 2015, 5:11 PM
Bastien Montagne (mont29) edited edge metadata.
  • 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

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

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
Bastien Montagne (mont29) updated this revision to Diff 3460.Feb 10 2015, 9:24 PM
Bastien Montagne (mont29) edited edge metadata.

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.

Bastien Montagne (mont29) updated this revision to Diff 3462.Feb 10 2015, 10:04 PM
Bastien Montagne (mont29) edited edge metadata.
  • 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
4160

(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
4160

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.

Bastien Montagne (mont29) updated this revision to Diff 3463.Feb 10 2015, 10:28 PM
  • Minor fixes.
Campbell Barton (campbellbarton) requested changes to this revision.Feb 10 2015, 10:41 PM
Campbell Barton (campbellbarton) edited edge metadata.

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
4160

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
Bastien Montagne (mont29) updated this revision to Diff 3464.Feb 10 2015, 11:07 PM
Bastien Montagne (mont29) edited edge metadata.

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.
Campbell Barton (campbellbarton) added a comment.Feb 10 2015, 11:19 PM
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)
Bastien Montagne (mont29) updated this revision to Diff 3465.Feb 10 2015, 11:31 PM
Bastien Montagne (mont29) edited edge metadata.
    

  • 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
Revision Contents
Changeset List
PathSize
release/
scripts/
startup/
bl_ui/
122 lines
source/
blender/
blenlib/
2 lines
intern/
71 lines
blenloader/
intern/
30 lines
editors/
include/
35 lines
interface/
44 lines
5 lines
4 lines
32 lines
screen/
2 lines
space_file/
2 lines
4 lines
175 lines
168 lines
2 lines
35 lines
195 lines
64 lines
makesdna/
11 lines
makesrna/
1 line
intern/
364 lines
blenderplayer/
bad_level_call_stubs/
9 lines
Diff 3454
View Options
release/scripts/startup/bl_ui/space_filebrowser.py
Loading...
View Options
source/blender/blenlib/BLI_linklist.h
Loading...
View Options
source/blender/blenlib/intern/BLI_linklist.c
Loading...
View Options
source/blender/blenloader/intern/versioning_270.c
Loading...
View Options
source/blender/editors/include/ED_fileselect.h
Loading...
View Options
source/blender/editors/interface/interface.c
Loading...
View Options
source/blender/editors/interface/interface_intern.h
Loading...
View Options
source/blender/editors/interface/interface_layout.c
Loading...
View Options
source/blender/editors/interface/interface_templates.c
Loading...
View Options
source/blender/editors/screen/area.c
Loading...
View Options
source/blender/editors/space_file/file_draw.c
Loading...
View Options
source/blender/editors/space_file/file_intern.h
Loading...
View Options
source/blender/editors/space_file/file_ops.c
Loading...
View Options
source/blender/editors/space_file/file_panels.c
Loading...
View Options
source/blender/editors/space_file/filesel.c
Loading...
View Options
source/blender/editors/space_file/fsmenu.h
Loading...
View Options
source/blender/editors/space_file/fsmenu.c
Loading...
View Options
source/blender/editors/space_file/space_file.c
Loading...
View Options
source/blender/makesdna/DNA_space_types.h
Loading...
View Options
source/blender/makesrna/RNA_access.h
Loading...
View Options
source/blender/makesrna/intern/rna_space.c
Loading...
View Options
source/blenderplayer/bad_level_call_stubs/stubs.c
Loading...