griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Refactor: Make asset catalog service non-public, access through API

Browse Source 
Making the member private (or at least protected) makes threat safety
more tangible, and we don't need to expose locking in the API. Generally
we need to make data more encapsulated, so we can make edits more
controlled and threat safe.

Asset libraries also always have a catalog-service, so it can accessed
by reference, rather than pointer that would have to be null-checked.
This commit is contained in:
Julian Eisel
2024-02-29 15:27:50 +01:00
parent efb511a76d
commit 4cc1c65272
13 changed files with 79 additions and 98 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
source/blender/asset_system/AS_asset_library.hh
View File

@@ -69,6 +69,8 @@ class AssetLibrary {
std::unique_ptr<AssetStorage> asset_storage_;
protected:
std::unique_ptr<AssetCatalogService> catalog_service_;
std::optional<eAssetImportMethod> import_method_;
/** Assets owned by this library may be imported with a different method than set in
* #import_method_ above, it's just a default. */
@@ -83,8 +85,6 @@ class AssetLibrary {
* for managing the "Save Catalog Changes" in the quit-confirmation dialog box. */
static bool save_catalogs_when_file_is_saved;
std::unique_ptr<AssetCatalogService> catalog_service;
friend class AssetLibraryService;
friend class AssetRepresentation;
@@ -111,6 +111,8 @@ class AssetLibrary {
void load_catalogs();
AssetCatalogService &catalog_service() const;
/**
* Create a representation of an asset to be considered part of this library. Once the
* representation is not needed anymore, it must be freed using #remove_asset(), or there will be

17
source/blender/asset_system/intern/asset_library.cc
View File

@@ -167,7 +167,7 @@ AssetLibrary::AssetLibrary(eAssetLibraryType library_type, StringRef name, Strin
name_(name),
root_path_(std::make_shared<std::string>(utils::normalize_directory_path(root_path))),
asset_storage_(std::make_unique<AssetStorage>()),
catalog_service(std::make_unique<AssetCatalogService>())
catalog_service_(std::make_unique<AssetCatalogService>())
{
}
@@ -189,7 +189,12 @@ void AssetLibrary::load_catalogs()
{
auto catalog_service = std::make_unique<AssetCatalogService>(root_path());
catalog_service->load_from_disk();
this->catalog_service = std::move(catalog_service);
this->catalog_service_ = std::move(catalog_service);
}
AssetCatalogService &AssetLibrary::catalog_service() const
{
return *catalog_service_;
}
void AssetLibrary::refresh_catalogs() {}
@@ -254,12 +259,8 @@ void AssetLibrary::on_blend_save_post(Main *main,
PointerRNA ** /*pointers*/,
const int /*num_pointers*/)
{
if (this->catalog_service == nullptr) {
return;
}
if (save_catalogs_when_file_is_saved) {
this->catalog_service->write_to_disk(main->filepath);
this->catalog_service().write_to_disk(main->filepath);
}
}
@@ -281,7 +282,7 @@ void AssetLibrary::refresh_catalog_simplename(AssetMetaData *asset_data)
asset_data->catalog_simple_name[0] = '\0';
return;
}
const AssetCatalog *catalog = this->catalog_service->find_catalog(asset_data->catalog_id);
const AssetCatalog *catalog = this->catalog_service().find_catalog(asset_data->catalog_id);
if (catalog == nullptr) {
/* No-op if the catalog cannot be found. This could be the kind of "the catalog definition file
* is corrupt/lost" scenario that the simple name is meant to help recover from. */

6
source/blender/asset_system/intern/asset_library_all.cc
View File

@@ -30,11 +30,11 @@ void AllAssetLibrary::rebuild_catalogs_from_nested(const bool reload_nested_cata
AssetLibrary::foreach_loaded(
[&](AssetLibrary &nested) {
if (reload_nested_catalogs) {
nested.catalog_service->reload_catalogs();
nested.catalog_service().reload_catalogs();
}
new_catalog_service->add_from_existing(
*nested.catalog_service,
nested.catalog_service(),
/*on_duplicate_items=*/[](const AssetCatalog &existing,
const AssetCatalog &to_be_ignored) {
if (existing.path == to_be_ignored.path) {
@@ -56,7 +56,7 @@ void AllAssetLibrary::rebuild_catalogs_from_nested(const bool reload_nested_cata
},
false);
this->catalog_service = std::move(new_catalog_service);
this->catalog_service_ = std::move(new_catalog_service);
catalogs_dirty_ = false;
}

2
source/blender/asset_system/intern/asset_library_on_disk.cc
View File

@@ -20,7 +20,7 @@ OnDiskAssetLibrary::OnDiskAssetLibrary(eAssetLibraryType library_type,
void OnDiskAssetLibrary::refresh_catalogs()
{
catalog_service->reload_catalogs();
catalog_service().reload_catalogs();
}
} // namespace blender::asset_system

2
source/blender/asset_system/intern/asset_library_service.cc
View File

@@ -503,7 +503,7 @@ bool AssetLibraryService::has_any_unsaved_catalogs() const
foreach_loaded_asset_library(
[&has_unsaved_changes](AssetLibrary &library) {
if (library.catalog_service->has_unsaved_changes()) {
if (library.catalog_service().has_unsaved_changes()) {
has_unsaved_changes = true;
}
},

20
source/blender/asset_system/tests/asset_library_service_test.cc
View File

@@ -173,10 +173,10 @@ TEST_F(AssetLibraryServiceTest, catalogs_loaded)
AssetLibraryService *const service = AssetLibraryService::get();
AssetLibrary *const lib = service->get_asset_library_on_disk_custom(__func__,
asset_library_root_);
AssetCatalogService *const cat_service = lib->catalog_service.get();
AssetCatalogService &cat_service = lib->catalog_service();
const bUUID UUID_POSES_ELLIE("df60e1f6-2259-475b-93d9-69a1b4a8db78");
EXPECT_NE(nullptr, cat_service->find_catalog(UUID_POSES_ELLIE))
EXPECT_NE(nullptr, cat_service.find_catalog(UUID_POSES_ELLIE))
<< "Catalogs should be loaded after getting an asset library from disk.";
}
@@ -188,21 +188,21 @@ TEST_F(AssetLibraryServiceTest, has_any_unsaved_catalogs)
AssetLibrary *const lib = service->get_asset_library_on_disk_custom(__func__,
asset_library_root_);
AssetCatalogService *const cat_service = lib->catalog_service.get();
AssetCatalogService &cat_service = lib->catalog_service();
EXPECT_FALSE(service->has_any_unsaved_catalogs())
<< "Unchanged AssetLibrary should have no unsaved catalogs";
const bUUID UUID_POSES_ELLIE("df60e1f6-2259-475b-93d9-69a1b4a8db78");
cat_service->prune_catalogs_by_id(UUID_POSES_ELLIE);
cat_service.prune_catalogs_by_id(UUID_POSES_ELLIE);
EXPECT_FALSE(service->has_any_unsaved_catalogs())
<< "Deletion of catalogs via AssetCatalogService should not automatically tag as 'unsaved "
"changes'.";
const bUUID UUID_POSES_RUZENA("79a4f887-ab60-4bd4-94da-d572e27d6aed");
AssetCatalog *cat = cat_service->find_catalog(UUID_POSES_RUZENA);
AssetCatalog *cat = cat_service.find_catalog(UUID_POSES_RUZENA);
ASSERT_NE(nullptr, cat) << "Catalog " << UUID_POSES_RUZENA << " should be known";
cat_service->tag_has_unsaved_changes(cat);
cat_service.tag_has_unsaved_changes(cat);
EXPECT_TRUE(service->has_any_unsaved_catalogs())
<< "Tagging as having unsaved changes of a single catalog service should result in unsaved "
"changes being reported.";
@@ -224,17 +224,17 @@ TEST_F(AssetLibraryServiceTest, has_any_unsaved_catalogs_after_write)
EXPECT_FALSE(service->has_any_unsaved_catalogs())
<< "Unchanged AssetLibrary should have no unsaved catalogs";
AssetCatalogService *const cat_service = lib->catalog_service.get();
AssetCatalog *cat = cat_service->find_catalog(UUID_POSES_ELLIE);
AssetCatalogService &cat_service = lib->catalog_service();
AssetCatalog *cat = cat_service.find_catalog(UUID_POSES_ELLIE);
cat_service->tag_has_unsaved_changes(cat);
cat_service.tag_has_unsaved_changes(cat);
EXPECT_TRUE(service->has_any_unsaved_catalogs())
<< "Tagging as having unsaved changes of a single catalog service should result in unsaved "
"changes being reported.";
EXPECT_TRUE(cat->flags.has_unsaved_changes);
cat_service->write_to_disk(writable_dir + "dummy_path.blend");
cat_service.write_to_disk(writable_dir + "dummy_path.blend");
EXPECT_FALSE(service->has_any_unsaved_catalogs())
<< "Written AssetCatalogService should have no unsaved catalogs";
EXPECT_FALSE(cat->flags.has_unsaved_changes);

11
source/blender/asset_system/tests/asset_library_test.cc
View File

@@ -47,14 +47,13 @@ TEST_F(AssetLibraryTest, AS_asset_library_load)
ASSERT_NE(nullptr, library);
/* Check that it can be cast to the C++ type and has a Catalog Service. */
AssetCatalogService *service = library->catalog_service.get();
ASSERT_NE(nullptr, service);
const AssetCatalogService &service = library->catalog_service();
/* Check that the catalogs defined in the library are actually loaded. This just tests one single
* catalog, as that indicates the file has been loaded. Testing that loading went OK is for
* the asset catalog service tests. */
const bUUID uuid_poses_ellie("df60e1f6-2259-475b-93d9-69a1b4a8db78");
AssetCatalog *poses_ellie = service->find_catalog(uuid_poses_ellie);
AssetCatalog *poses_ellie = service.find_catalog(uuid_poses_ellie);
ASSERT_NE(nullptr, poses_ellie) << "unable to find POSES_ELLIE catalog";
EXPECT_EQ("character/Ellie/poselib", poses_ellie->path.str());
}
@@ -73,11 +72,9 @@ TEST_F(AssetLibraryTest, load_nonexistent_directory)
ASSERT_NE(nullptr, library);
/* Check that it can be cast to the C++ type and has a Catalog Service. */
AssetCatalogService *service = library->catalog_service.get();
ASSERT_NE(nullptr, service);
AssetCatalogService &service = library->catalog_service();
/* Check that the catalog service doesn't have any catalogs. */
EXPECT_TRUE(service->is_empty());
EXPECT_TRUE(service.is_empty());
}
} // namespace blender::asset_system::tests

81
source/blender/editors/asset/intern/asset_catalog.cc
View File

@@ -28,8 +28,8 @@ using namespace blender::asset_system;
bool catalogs_read_only(const AssetLibrary &library)
{
asset_system::AssetCatalogService *catalog_service = library.catalog_service.get();
return catalog_service->is_read_only();
const asset_system::AssetCatalogService &catalog_service = library.catalog_service();
return catalog_service.is_read_only();
}
struct CatalogUniqueNameFnData {
@@ -61,23 +61,20 @@ asset_system::AssetCatalog *catalog_add(AssetLibrary *library,
StringRefNull name,
StringRef parent_path)
{
asset_system::AssetCatalogService *catalog_service = library->catalog_service.get();
if (!catalog_service) {
return nullptr;
}
if (catalog_service->is_read_only()) {
asset_system::AssetCatalogService &catalog_service = library->catalog_service();
if (catalog_service.is_read_only()) {
return nullptr;
}
std::string unique_name = catalog_name_ensure_unique(*catalog_service, name, parent_path);
std::string unique_name = catalog_name_ensure_unique(catalog_service, name, parent_path);
AssetCatalogPath fullpath = AssetCatalogPath(parent_path) / unique_name;
catalog_service->undo_push();
asset_system::AssetCatalog *new_catalog = catalog_service->create_catalog(fullpath);
catalog_service.undo_push();
asset_system::AssetCatalog *new_catalog = catalog_service.create_catalog(fullpath);
if (!new_catalog) {
return nullptr;
}
catalog_service->tag_has_unsaved_changes(new_catalog);
catalog_service.tag_has_unsaved_changes(new_catalog);
WM_main_add_notifier(NC_SPACE | ND_SPACE_ASSET_PARAMS, nullptr);
return new_catalog;
@@ -85,18 +82,14 @@ asset_system::AssetCatalog *catalog_add(AssetLibrary *library,
void catalog_remove(AssetLibrary *library, const CatalogID &catalog_id)
{
asset_system::AssetCatalogService *catalog_service = library->catalog_service.get();
if (!catalog_service) {
BLI_assert_unreachable();
return;
}
if (catalog_service->is_read_only()) {
asset_system::AssetCatalogService &catalog_service = library->catalog_service();
if (catalog_service.is_read_only()) {
return;
}
catalog_service->undo_push();
catalog_service->tag_has_unsaved_changes(nullptr);
catalog_service->prune_catalogs_by_id(catalog_id);
catalog_service.undo_push();
catalog_service.tag_has_unsaved_changes(nullptr);
catalog_service.prune_catalogs_by_id(catalog_id);
WM_main_add_notifier(NC_SPACE | ND_SPACE_ASSET_PARAMS, nullptr);
}
@@ -104,16 +97,12 @@ void catalog_rename(AssetLibrary *library,
const CatalogID catalog_id,
const StringRefNull new_name)
{
asset_system::AssetCatalogService *catalog_service = library->catalog_service.get();
if (!catalog_service) {
BLI_assert_unreachable();
return;
}
if (catalog_service->is_read_only()) {
asset_system::AssetCatalogService &catalog_service = library->catalog_service();
if (catalog_service.is_read_only()) {
return;
}
AssetCatalog *catalog = catalog_service->find_catalog(catalog_id);
AssetCatalog *catalog = catalog_service.find_catalog(catalog_id);
const AssetCatalogPath new_path = catalog->path.parent() / StringRef(new_name);
const AssetCatalogPath clean_new_path = new_path.cleanup();
@@ -123,9 +112,9 @@ void catalog_rename(AssetLibrary *library,
return;
}
catalog_service->undo_push();
catalog_service->tag_has_unsaved_changes(catalog);
catalog_service->update_catalog_path(catalog_id, clean_new_path);
catalog_service.undo_push();
catalog_service.tag_has_unsaved_changes(catalog);
catalog_service.update_catalog_path(catalog_id, clean_new_path);
WM_main_add_notifier(NC_SPACE | ND_SPACE_ASSET_PARAMS, nullptr);
}
@@ -133,22 +122,18 @@ void catalog_move(AssetLibrary *library,
const CatalogID src_catalog_id,
const std::optional<CatalogID> dst_parent_catalog_id)
{
asset_system::AssetCatalogService *catalog_service = library->catalog_service.get();
if (!catalog_service) {
BLI_assert_unreachable();
return;
}
if (catalog_service->is_read_only()) {
asset_system::AssetCatalogService &catalog_service = library->catalog_service();
if (catalog_service.is_read_only()) {
return;
}
AssetCatalog *src_catalog = catalog_service->find_catalog(src_catalog_id);
AssetCatalog *src_catalog = catalog_service.find_catalog(src_catalog_id);
if (!src_catalog) {
BLI_assert_unreachable();
return;
}
AssetCatalog *dst_catalog = dst_parent_catalog_id ?
catalog_service->find_catalog(*dst_parent_catalog_id) :
catalog_service.find_catalog(*dst_parent_catalog_id) :
nullptr;
if (!dst_catalog && dst_parent_catalog_id) {
BLI_assert_unreachable();
@@ -156,7 +141,7 @@ void catalog_move(AssetLibrary *library,
}
std::string unique_name = catalog_name_ensure_unique(
*catalog_service, src_catalog->path.name(), dst_catalog ? dst_catalog->path.c_str() : "");
catalog_service, src_catalog->path.name(), dst_catalog ? dst_catalog->path.c_str() : "");
/* If a destination catalog was given, construct the path using that. Otherwise, the path is just
* the name of the catalog to be moved, which means it ends up at the root level. */
const AssetCatalogPath new_path = dst_catalog ? (dst_catalog->path / unique_name) :
@@ -168,27 +153,23 @@ void catalog_move(AssetLibrary *library,
return;
}
catalog_service->undo_push();
catalog_service->tag_has_unsaved_changes(src_catalog);
catalog_service->update_catalog_path(src_catalog_id, clean_new_path);
catalog_service.undo_push();
catalog_service.tag_has_unsaved_changes(src_catalog);
catalog_service.update_catalog_path(src_catalog_id, clean_new_path);
WM_main_add_notifier(NC_SPACE | ND_SPACE_ASSET_PARAMS, nullptr);
}
void catalogs_save_from_main_path(AssetLibrary *library, const Main *bmain)
{
asset_system::AssetCatalogService *catalog_service = library->catalog_service.get();
if (!catalog_service) {
BLI_assert_unreachable();
return;
}
if (catalog_service->is_read_only()) {
asset_system::AssetCatalogService &catalog_service = library->catalog_service();
if (catalog_service.is_read_only()) {
return;
}
/* Since writing to disk also means loading any on-disk changes, it may be a good idea to store
* an undo step. */
catalog_service->undo_push();
catalog_service->write_to_disk(bmain->filepath);
catalog_service.undo_push();
catalog_service.write_to_disk(bmain->filepath);
}
void catalogs_set_save_catalogs_when_file_is_saved(const bool should_save)

12
source/blender/editors/asset/intern/asset_filter.cc
View File

@@ -64,7 +64,7 @@ asset_system::AssetCatalogTree build_filtered_catalog_tree(
return true;
}
const asset_system::AssetCatalog *catalog = library.catalog_service->find_catalog(
const asset_system::AssetCatalog *catalog = library.catalog_service().find_catalog(
meta_data.catalog_id);
if (catalog == nullptr) {
return true;
@@ -75,13 +75,13 @@ asset_system::AssetCatalogTree build_filtered_catalog_tree(
/* Build catalog tree. */
asset_system::AssetCatalogTree filtered_tree;
const asset_system::AssetCatalogTree &full_tree = library.catalog_service->catalog_tree();
const asset_system::AssetCatalogTree &full_tree = library.catalog_service().catalog_tree();
full_tree.foreach_item([&](const asset_system::AssetCatalogTreeItem &item) {
if (!known_paths.contains(item.catalog_path().str())) {
return;
}
asset_system::AssetCatalog *catalog = library.catalog_service->find_catalog(
asset_system::AssetCatalog *catalog = library.catalog_service().find_catalog(
item.get_catalog_id());
if (catalog == nullptr) {
return;
@@ -122,7 +122,7 @@ AssetItemTree build_filtered_all_catalog_tree(
return true;
}
const asset_system::AssetCatalog *catalog = library->catalog_service->find_catalog(
const asset_system::AssetCatalog *catalog = library->catalog_service().find_catalog(
meta_data.catalog_id);
if (catalog == nullptr) {
/* Also include assets with catalogs we're unable to find (e.g. the catalog was deleted) in
@@ -135,12 +135,12 @@ AssetItemTree build_filtered_all_catalog_tree(
});
asset_system::AssetCatalogTree catalogs_with_node_assets;
const asset_system::AssetCatalogTree &catalog_tree = library->catalog_service->catalog_tree();
const asset_system::AssetCatalogTree &catalog_tree = library->catalog_service().catalog_tree();
catalog_tree.foreach_item([&](const asset_system::AssetCatalogTreeItem &item) {
if (assets_per_path.lookup(item.catalog_path()).is_empty()) {
return;
}
asset_system::AssetCatalog *catalog = library->catalog_service->find_catalog(
asset_system::AssetCatalog *catalog = library->catalog_service().find_catalog(
item.get_catalog_id());
if (catalog == nullptr) {
return;

2
source/blender/editors/asset/intern/asset_menu_utils.cc
View File

@@ -140,7 +140,7 @@ PointerRNA persistent_catalog_path_rna_pointer(const bScreen &owner_screen,
const asset_system::AssetLibrary &library,
const asset_system::AssetCatalogTreeItem &item)
{
const asset_system::AssetCatalog *catalog = library.catalog_service->find_catalog_by_path(
const asset_system::AssetCatalog *catalog = library.catalog_service().find_catalog_by_path(
item.catalog_path());
if (!catalog) {
return PointerRNA_NULL;

2
source/blender/editors/asset/intern/asset_ops.cc
View File

@@ -587,7 +587,7 @@ static asset_system::AssetCatalogService *get_catalog_service(bContext *C)
}
asset_system::AssetLibrary *asset_lib = ED_fileselect_active_asset_library_get(sfile);
return asset_lib->catalog_service.get();
return &asset_lib->catalog_service();
}
static int asset_catalog_undo_exec(bContext *C, wmOperator * /*op*/)

4
source/blender/editors/asset/intern/asset_shelf_asset_view.cc
View File

@@ -166,13 +166,13 @@ static std::optional<asset_system::AssetCatalogFilter> catalog_filter_from_shelf
return {};
}
asset_system::AssetCatalog *active_catalog = library.catalog_service->find_catalog_by_path(
asset_system::AssetCatalog *active_catalog = library.catalog_service().find_catalog_by_path(
shelf_settings.active_catalog_path);
if (!active_catalog) {
return {};
}
return library.catalog_service->create_catalog_filter(active_catalog->catalog_id);
return library.catalog_service().create_catalog_filter(active_catalog->catalog_id);
}
/* ---------------------------------------------------------------------- */

12
source/blender/editors/space_file/asset_catalog_tree_view.cc
View File

@@ -183,8 +183,8 @@ AssetCatalogTreeView::AssetCatalogTreeView(asset_system::AssetLibrary *library,
SpaceFile &space_file)
: asset_library_(library), params_(params), space_file_(space_file)
{
if (library && library->catalog_service) {
catalog_tree_ = &library->catalog_service->catalog_tree();
if (library) {
catalog_tree_ = &library->catalog_service().catalog_tree();
}
else {
catalog_tree_ = nullptr;
@@ -503,10 +503,10 @@ AssetCatalog *AssetCatalogDropTarget::get_drag_catalog(
if (drag.type != WM_DRAG_ASSET_CATALOG) {
return nullptr;
}
const AssetCatalogService *catalog_service = asset_library.catalog_service.get();
const AssetCatalogService &catalog_service = asset_library.catalog_service();
const wmDragAssetCatalog *catalog_drag = WM_drag_get_asset_catalog_data(&drag);
return catalog_service->find_catalog(catalog_drag->drag_catalog_id);
return catalog_service.find_catalog(catalog_drag->drag_catalog_id);
}
bool AssetCatalogDropTarget::has_droppable_asset(const wmDrag &drag, const char **r_disabled_hint)
@@ -744,11 +744,11 @@ void file_ensure_updated_catalog_filter_data(
{
AssetCatalogFilterSettings *filter_settings = reinterpret_cast<AssetCatalogFilterSettings *>(
filter_settings_handle);
const AssetCatalogService *catalog_service = asset_library->catalog_service.get();
const AssetCatalogService &catalog_service = asset_library->catalog_service();
if (filter_settings->asset_catalog_visibility != FILE_SHOW_ASSETS_ALL_CATALOGS) {
filter_settings->catalog_filter = std::make_unique<AssetCatalogFilter>(
catalog_service->create_catalog_filter(filter_settings->asset_catalog_id));
catalog_service.create_catalog_filter(filter_settings->asset_catalog_id));
}
}