Page MenuHome
Differential D11119

Assets: Keep assets active after renaming, ensure they are scrolled into view
ClosedPublic
Actions

Authored by Julian Eisel (Severin) on Apr 29 2021, 6:25 PM.

Details

Reviewers
Sybren A. Stüvel (sybren)
Commits
rBf4cb3ccd9c07: Assets: Keep assets active after renaming, ensure they are scrolled into view
Summary

When renaming an ID somewhere in the UI after marking it as asset, it would often get lost in the Asset Browser (scrolled out of view). It would also get deactivated.
This patch makes sure that if an asset is active whose ID gets renamed, it is kept active and visible. That is important for a fast, uninterrupted asset creation workflow, where users often rename assets while working in the asset browser.

Old code stored the new file-name to identify a file after re-reading the file-list after the rename. For assets that doesn't work because there may be multiple assets with the same name. Here the simple solution of just storing the pointer to the renamed ID is chosen, rather than relying on the file-name in this case. (Should be fine with undo, since the ID * reference is short lived, it's not stored over possible undo steps. If it turns out to have issues, I rather switch to a rename_id_uuid, but keep that separate from the file->uid).

  • The smooth-scrolling seems to have some issues in master already. Fixing that is a separate thing to do.
  • Partially includes D9994. These changes were needed for this to work.

Diff Detail

Repository
rB Blender
Branch
temp-asset-browser-rename-keep-active (branched from master)
Build Status
Buildable 15584
Build 15584: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) requested review of this revision.Apr 29 2021, 6:25 PM
Julian Eisel (Severin) created this revision.
Harbormaster completed remote builds in B14308: Diff 36660.Apr 29 2021, 6:25 PM
Julian Eisel (Severin) planned changes to this revision.Apr 29 2021, 6:26 PM
Julian Eisel (Severin) added projects: Asset Browser (Archived), Pipeline, Assets & I/O, User Interface.

Just want to have this here already in case somebody wants to give early feedback. But I'm planning to do a polish pass still, split off some changes and give a proper patch description.

Julian Eisel (Severin) retitled this revision from Asset: Keep assets active after renaming, make sure they are scrolled into view to Assets: Keep assets active after renaming, ensure they are scrolled into view.Apr 29 2021, 6:29 PM
Pieter Schiettecatte (Schiette) added a subscriber: Pieter Schiettecatte (Schiette).Apr 29 2021, 10:38 PM
Julian Eisel (Severin) mentioned this in rB01e1944cd455: File Browser: Refactor file "UUID" code (which wasn't really a "UUID").Jul 2 2021, 1:52 PM
Julian Eisel (Severin) updated this revision to Diff 39088.EditedJul 3 2021, 11:12 PM
  • Bunch of updates, use ID pointer rather than UID to reference file

Even though I don't find this all too nice, it should be safer to use the ID pointer directly rather than the ID's session UUID. Mostly because it makes it possible to mix regular files with ID files (files representing a local ID asset).
The pointer storage is short lived and shouldn't cause any trouble over undo steps.

Harbormaster completed remote builds in B15584: Diff 39088.Jul 3 2021, 11:12 PM
Julian Eisel (Severin) updated this revision to Diff 39089.Jul 3 2021, 11:23 PM
  • Remove unnecessary changes
Harbormaster completed remote builds in B15585: Diff 39089.Jul 3 2021, 11:23 PM
Julian Eisel (Severin) updated this revision to Diff 39151.Jul 5 2021, 4:14 PM
  • Committed some smaller changes to master already
  • Remove problematic change in smooth-view logic. Caused issues, but it doesn't work well in master either.
Harbormaster completed remote builds in B15613: Diff 39151.Jul 5 2021, 4:14 PM
Julian Eisel (Severin) updated this revision to Diff 39156.Jul 5 2021, 4:38 PM
  • Remove wrong asserts.
Harbormaster completed remote builds in B15618: Diff 39156.Jul 5 2021, 4:38 PM
Julian Eisel (Severin) updated this revision to Diff 39159.Jul 5 2021, 5:19 PM
  • More (rather unrelated) changes committed directly to master
Harbormaster completed remote builds in B15619: Diff 39159.Jul 5 2021, 5:19 PM
Julian Eisel (Severin) added a reviewer: Sybren A. Stüvel (sybren).Jul 5 2021, 5:29 PM

Should be ready for review now.

I may split this up in two commits still, one to keep files active after renaming generally (see D9994: File Browser: select files and directories after renaming), one to do the same for local assets plus making sure they are in view.

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Jul 5 2021, 5:35 PM
Julian Eisel (Severin) updated this revision to Diff 39166.Jul 5 2021, 5:40 PM
  • Add & correct comments
Harbormaster completed remote builds in B15622: Diff 39166.Jul 5 2021, 5:41 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Jul 6 2021, 11:36 AM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/editors/space_file/file_ops.c
2346

Maybe turn this into BLI_assert(params->rename_id == NULL || !"explanation of what is wrong");, because I can't just see from this line why the assertion is of any importance.

source/blender/editors/space_file/filesel.c
1269–1275

This should be extracted to a separate function, something like const int idx = filelist_file_find_renamed(sfile->files, params).

source/blender/editors/space_file/space_file.c
476

This is a personal thing, so feel free to ignore, but I'd find if (active_file_id && (wmn->reference == active_file_id)) easier to read. Due to the preceding line, the thing on my mind is active_file_id.

Both conditions are equivalent, because when (wmn->reference == active_file_id) is true, any comparison on wmn->reference is equivalent to that comparison on active_file_id.

source/blender/makesdna/DNA_space_types.h
731–733

Would it help to explain that this is about renaming local assets? Or do you think "ID file" is a concept that'll be used beyond the scope of assets?

This revision now requires changes to proceed.Jul 6 2021, 11:36 AM
Julian Eisel (Severin) updated this revision to Diff 39211.Jul 6 2021, 5:15 PM
  • Update with review feedback
Harbormaster completed remote builds in B15645: Diff 39211.Jul 6 2021, 5:15 PM
Julian Eisel (Severin) marked 4 inline comments as done.Jul 6 2021, 5:55 PM
Julian Eisel (Severin) added inline comments.
source/blender/editors/space_file/filesel.c
1269–1275

I used a file_params_ function because the decision over what file identifier to use (renamefile or rename_id) is to be made on the file-select-params level, the file-list doesn't deal with such stuff.

source/blender/makesdna/DNA_space_types.h
731–733

It may be, there is some old disabled code for displaying main contents. But maybe that was written before the Outliner was introduced. Think we can safely assume that it's gonna be just asset files for now, made it a bit more clear.

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Jul 6 2021, 5:56 PM
Sybren A. Stüvel (sybren) accepted this revision.Jul 6 2021, 6:32 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/editors/space_file/filesel.c
1269–1275

👍

This revision is now accepted and ready to land.Jul 6 2021, 6:32 PM
Julian Eisel (Severin) mentioned this in rB6b0869039a40: File Browser: Select files and directories after renaming.Jul 7 2021, 6:44 PM
Julian Eisel (Severin) mentioned this in D9994: File Browser: select files and directories after renaming.Jul 7 2021, 6:50 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Jul 7 2021, 7:11 PM
Julian Eisel (Severin) updated this revision to Diff 39284.EditedJul 7 2021, 7:16 PM
Julian Eisel (Severin) marked 2 inline comments as done.
  • Merge master. I've committed 6b0869039a40 as alternative version to D9994, which this patch depended on. (This doesn't add anything new to this particular patch.)
Harbormaster completed remote builds in B15678: Diff 39284.Jul 7 2021, 7:16 PM
Closed by commit rBf4cb3ccd9c07: Assets: Keep assets active after renaming, ensure they are scrolled into view (authored by Julian Eisel (Severin)). · Explain WhyJul 7 2021, 7:27 PM
This revision was automatically updated to reflect the committed changes.
Julian Eisel (Severin) added a commit: rBf4cb3ccd9c07: Assets: Keep assets active after renaming, ensure they are scrolled into view.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
intern/
11 lines
editors/
space_file/
13 lines
4 lines
19 lines
5 lines
41 lines
42 lines
17 lines
makesdna/
8 lines

Diff 39088

View Options

source/blender/blenkernel/intern/screen.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/filelist.h

Loading...
View Options

source/blender/editors/space_file/filelist.c

Loading...
View Options

source/blender/editors/space_file/filesel.c

Loading...
View Options

source/blender/editors/space_file/space_file.c

Loading...
View Options

source/blender/makesdna/DNA_space_types.h

Loading...