diff options
author | Sybren A. Stüvel <sybren@blender.org> | 2021-10-19 19:07:22 +0300 |
---|---|---|
committer | Sybren A. Stüvel <sybren@blender.org> | 2021-10-19 19:07:22 +0300 |
commit | 823996b0342b7352fc5b2e24eceb6204612438cd (patch) | |
tree | 08dca1ce5386c2b06ab01fad1f2f1ac5c236047a /source/blender/blenkernel | |
parent | b6c3b41d413813d8059476f2c0357b7a4e51ad22 (diff) |
Asset Browser: Improved workflow for asset catalog saving
No longer save asset catalogs on blendfile save. Instead:
- extend the confirmation prompt for unsaved changes to show unsaved
catalogs.
- In the confirmation prompt, make catalog saving explicit & optional,
just like we do it for external images. {F10881736}
- In the Asset Browser catalog tree, show an operator icon to save the
catalogs to disk. It's grayed out if there are no changes to save, or
if the .blend wasn't saved yet (required to know where to save the
catalog definitions to). {F10881743}
Much of the work was done by @Severin and reviewed by me, then we
swapped roles.
Reviewed By: Severin
Differential Revision: https://developer.blender.org/D12796
Diffstat (limited to 'source/blender/blenkernel')
10 files changed, 176 insertions, 31 deletions
diff --git a/source/blender/blenkernel/BKE_asset_catalog.hh b/source/blender/blenkernel/BKE_asset_catalog.hh index d5c8acff960..f7896e0f51e 100644 --- a/source/blender/blenkernel/BKE_asset_catalog.hh +++ b/source/blender/blenkernel/BKE_asset_catalog.hh @@ -63,13 +63,21 @@ class AssetCatalogService { AssetCatalogService(); explicit AssetCatalogService(const CatalogFilePath &asset_library_root); + /** + * Set global tag indicating that some catalog modifications are unsaved that could get lost + * on exit. This tag is not set by internal catalog code, the catalog service user is responsible + * for it. It is cleared by #write_to_disk(). + */ + void tag_has_unsaved_changes(); + bool has_unsaved_changes() const; + /** Load asset catalog definitions from the files found in the asset library. */ void load_from_disk(); /** Load asset catalog definitions from the given file or directory. */ void load_from_disk(const CatalogFilePath &file_or_directory_path); /** - * Write the catalog definitions to disk in response to the blend file being saved. + * Write the catalog definitions to disk. * * The location where the catalogs are saved is variable, and depends on the location of the * blend file. The first matching rule wins: @@ -85,7 +93,7 @@ class AssetCatalogService { * * Return true on success, which either means there were no in-memory categories to save, * or the save was successful. */ - bool write_to_disk_on_blendfile_save(const CatalogFilePath &blend_file_path); + bool write_to_disk(const CatalogFilePath &blend_file_path); /** * Merge on-disk changes into the in-memory asset catalogs. @@ -166,10 +174,15 @@ class AssetCatalogService { Vector<std::unique_ptr<AssetCatalogCollection>> undo_snapshots_; Vector<std::unique_ptr<AssetCatalogCollection>> redo_snapshots_; + bool has_unsaved_changes_ = false; void load_directory_recursive(const CatalogFilePath &directory_path); void load_single_file(const CatalogFilePath &catalog_definition_file_path); + /** Implementation of #write_to_disk() that doesn't clear the "has unsaved changes" tag. */ + bool write_to_disk_ex(const CatalogFilePath &blend_file_path); + void untag_has_unsaved_changes(); + std::unique_ptr<AssetCatalogDefinitionFile> parse_catalog_file( const CatalogFilePath &catalog_definition_file_path); diff --git a/source/blender/blenkernel/BKE_asset_library.h b/source/blender/blenkernel/BKE_asset_library.h index b4674a8d932..ca12fd6f4fb 100644 --- a/source/blender/blenkernel/BKE_asset_library.h +++ b/source/blender/blenkernel/BKE_asset_library.h @@ -77,6 +77,9 @@ bool BKE_asset_library_find_suitable_root_path_from_main( void BKE_asset_library_refresh_catalog_simplename(struct AssetLibrary *asset_library, struct AssetMetaData *asset_data); +/** Return whether any loaded AssetLibrary has unsaved changes to its catalogs. */ +bool BKE_asset_library_has_any_unsaved_catalogs(void); + #ifdef __cplusplus } #endif diff --git a/source/blender/blenkernel/BKE_asset_library.hh b/source/blender/blenkernel/BKE_asset_library.hh index 6d5c9836b26..64c8afecaef 100644 --- a/source/blender/blenkernel/BKE_asset_library.hh +++ b/source/blender/blenkernel/BKE_asset_library.hh @@ -38,6 +38,10 @@ namespace blender::bke { * For now this is only for catalogs, later this can be expanded to indexes/caches/more. */ struct AssetLibrary { + /* Controlled by #ED_asset_catalogs_set_save_catalogs_when_file_is_saved, + * 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; AssetLibrary(); @@ -53,10 +57,10 @@ struct AssetLibrary { * meant to help recover from. */ void refresh_catalog_simplename(struct AssetMetaData *asset_data); - void on_save_handler_register(); - void on_save_handler_unregister(); + void on_blend_save_handler_register(); + void on_blend_save_handler_unregister(); - void on_save_post(struct Main *, struct PointerRNA **pointers, const int num_pointers); + void on_blend_save_post(struct Main *, struct PointerRNA **pointers, const int num_pointers); private: bCallbackFuncStore on_save_callback_store_{}; diff --git a/source/blender/blenkernel/intern/asset_catalog.cc b/source/blender/blenkernel/intern/asset_catalog.cc index f3a4f88ef38..aa8f12d0e23 100644 --- a/source/blender/blenkernel/intern/asset_catalog.cc +++ b/source/blender/blenkernel/intern/asset_catalog.cc @@ -66,6 +66,21 @@ AssetCatalogService::AssetCatalogService(const CatalogFilePath &asset_library_ro { } +void AssetCatalogService::tag_has_unsaved_changes() +{ + has_unsaved_changes_ = true; +} + +void AssetCatalogService::untag_has_unsaved_changes() +{ + has_unsaved_changes_ = false; +} + +bool AssetCatalogService::has_unsaved_changes() const +{ + return has_unsaved_changes_; +} + bool AssetCatalogService::is_empty() const { return catalog_collection_->catalogs_.is_empty(); @@ -344,7 +359,17 @@ void AssetCatalogService::merge_from_disk_before_writing() cdf->parse_catalog_file(cdf->file_path, catalog_parsed_callback); } -bool AssetCatalogService::write_to_disk_on_blendfile_save(const CatalogFilePath &blend_file_path) +bool AssetCatalogService::write_to_disk(const CatalogFilePath &blend_file_path) +{ + if (!write_to_disk_ex(blend_file_path)) { + return false; + } + + untag_has_unsaved_changes(); + return true; +} + +bool AssetCatalogService::write_to_disk_ex(const CatalogFilePath &blend_file_path) { /* TODO(Sybren): expand to support multiple CDFs. */ diff --git a/source/blender/blenkernel/intern/asset_catalog_test.cc b/source/blender/blenkernel/intern/asset_catalog_test.cc index d1413d521a1..d743d250c1d 100644 --- a/source/blender/blenkernel/intern/asset_catalog_test.cc +++ b/source/blender/blenkernel/intern/asset_catalog_test.cc @@ -222,7 +222,7 @@ class AssetCatalogTest : public testing::Test { const AssetCatalog *cat = service.create_catalog("some/catalog/path"); /* Mock that the blend file is written to the directory already containing a CDF. */ - ASSERT_TRUE(service.write_to_disk_on_blendfile_save(blendfilename)); + ASSERT_TRUE(service.write_to_disk(blendfilename)); /* Test that the CDF still exists in the expected location. */ EXPECT_TRUE(BLI_exists(cdf_toplevel.c_str())); @@ -489,7 +489,7 @@ TEST_F(AssetCatalogTest, no_writing_empty_files) { const CatalogFilePath temp_lib_root = create_temp_path(); AssetCatalogService service(temp_lib_root); - service.write_to_disk_on_blendfile_save(temp_lib_root + "phony.blend"); + service.write_to_disk(temp_lib_root + "phony.blend"); const CatalogFilePath default_cdf_path = temp_lib_root + AssetCatalogService::DEFAULT_CATALOG_FILENAME; @@ -515,7 +515,7 @@ TEST_F(AssetCatalogTest, on_blendfile_save__with_existing_cdf) const AssetCatalog *cat = service.create_catalog("some/catalog/path"); const CatalogFilePath blendfilename = top_level_dir + "subdir/some_file.blend"; - ASSERT_TRUE(service.write_to_disk_on_blendfile_save(blendfilename)); + ASSERT_TRUE(service.write_to_disk(blendfilename)); EXPECT_EQ(cdf_filename, service.get_catalog_definition_file()->file_path); /* Test that the CDF was created in the expected location. */ @@ -542,7 +542,7 @@ TEST_F(AssetCatalogTest, on_blendfile_save__from_memory_into_empty_directory) const AssetCatalog *cat = service.create_catalog("some/catalog/path"); const CatalogFilePath blendfilename = target_dir + "some_file.blend"; - ASSERT_TRUE(service.write_to_disk_on_blendfile_save(blendfilename)); + ASSERT_TRUE(service.write_to_disk(blendfilename)); /* Test that the CDF was created in the expected location. */ const CatalogFilePath expected_cdf_path = target_dir + @@ -575,7 +575,7 @@ TEST_F(AssetCatalogTest, on_blendfile_save__from_memory_into_existing_cdf_and_me /* Mock that the blend file is written to a subdirectory of the asset library. */ const CatalogFilePath blendfilename = target_dir + "some_file.blend"; - ASSERT_TRUE(service.write_to_disk_on_blendfile_save(blendfilename)); + ASSERT_TRUE(service.write_to_disk(blendfilename)); /* Test that the CDF still exists in the expected location. */ const CatalogFilePath backup_filename = writable_cdf_file + "~"; @@ -630,7 +630,7 @@ TEST_F(AssetCatalogTest, create_first_catalog_from_scratch) EXPECT_FALSE(BLI_exists(temp_lib_root.c_str())); /* Writing to disk should create the directory + the default file. */ - service.write_to_disk_on_blendfile_save(temp_lib_root + "phony.blend"); + service.write_to_disk(temp_lib_root + "phony.blend"); EXPECT_TRUE(BLI_is_dir(temp_lib_root.c_str())); const CatalogFilePath definition_file_path = temp_lib_root + "/" + @@ -681,7 +681,7 @@ TEST_F(AssetCatalogTest, create_catalog_after_loading_file) << "expecting newly added catalog to not yet be saved to " << temp_lib_root; /* Write and reload the catalog file. */ - service.write_to_disk_on_blendfile_save(temp_lib_root + "phony.blend"); + service.write_to_disk(temp_lib_root + "phony.blend"); AssetCatalogService reloaded_service(temp_lib_root); reloaded_service.load_from_disk(); EXPECT_NE(nullptr, reloaded_service.find_catalog(UUID_POSES_ELLIE)) @@ -867,7 +867,7 @@ TEST_F(AssetCatalogTest, merge_catalog_files) ASSERT_EQ(0, BLI_copy(modified_cdf_file.c_str(), temp_cdf_file.c_str())); /* Overwrite the modified file. This should merge the on-disk file with our catalogs. */ - service.write_to_disk_on_blendfile_save(cdf_dir + "phony.blend"); + service.write_to_disk(cdf_dir + "phony.blend"); AssetCatalogService loaded_service(cdf_dir); loaded_service.load_from_disk(); @@ -897,7 +897,7 @@ TEST_F(AssetCatalogTest, backups) AssetCatalogService service(cdf_dir); service.load_from_disk(); service.delete_catalog_by_id(UUID_POSES_ELLIE); - service.write_to_disk_on_blendfile_save(cdf_dir + "phony.blend"); + service.write_to_disk(cdf_dir + "phony.blend"); const CatalogFilePath backup_path = writable_cdf_file + "~"; ASSERT_TRUE(BLI_is_file(backup_path.c_str())); diff --git a/source/blender/blenkernel/intern/asset_library.cc b/source/blender/blenkernel/intern/asset_library.cc index 32e7aab235d..6c4660ae75d 100644 --- a/source/blender/blenkernel/intern/asset_library.cc +++ b/source/blender/blenkernel/intern/asset_library.cc @@ -35,6 +35,8 @@ #include <memory> +bool blender::bke::AssetLibrary::save_catalogs_when_file_is_saved = true; + /** * Loading an asset library at this point only means loading the catalogs. Later on this should * invoke reading of asset representations too. @@ -52,6 +54,12 @@ struct AssetLibrary *BKE_asset_library_load(const char *library_path) return reinterpret_cast<struct AssetLibrary *>(lib); } +bool BKE_asset_library_has_any_unsaved_catalogs() +{ + blender::bke::AssetLibraryService *service = blender::bke::AssetLibraryService::get(); + return service->has_any_unsaved_catalogs(); +} + bool BKE_asset_library_find_suitable_root_path_from_path(const char *input_path, char *r_library_path) { @@ -109,7 +117,7 @@ AssetLibrary::AssetLibrary() : catalog_service(std::make_unique<AssetCatalogServ AssetLibrary::~AssetLibrary() { if (on_save_callback_store_.func) { - on_save_handler_unregister(); + on_blend_save_handler_unregister(); } } @@ -127,11 +135,12 @@ void asset_library_on_save_post(struct Main *main, void *arg) { AssetLibrary *asset_lib = static_cast<AssetLibrary *>(arg); - asset_lib->on_save_post(main, pointers, num_pointers); + asset_lib->on_blend_save_post(main, pointers, num_pointers); } + } // namespace -void AssetLibrary::on_save_handler_register() +void AssetLibrary::on_blend_save_handler_register() { /* The callback system doesn't own `on_save_callback_store_`. */ on_save_callback_store_.alloc = false; @@ -142,22 +151,24 @@ void AssetLibrary::on_save_handler_register() BKE_callback_add(&on_save_callback_store_, BKE_CB_EVT_SAVE_POST); } -void AssetLibrary::on_save_handler_unregister() +void AssetLibrary::on_blend_save_handler_unregister() { BKE_callback_remove(&on_save_callback_store_, BKE_CB_EVT_SAVE_POST); on_save_callback_store_.func = nullptr; on_save_callback_store_.arg = nullptr; } -void AssetLibrary::on_save_post(struct Main *main, - struct PointerRNA ** /*pointers*/, - const int /*num_pointers*/) +void AssetLibrary::on_blend_save_post(struct Main *main, + struct PointerRNA ** /*pointers*/, + const int /*num_pointers*/) { if (this->catalog_service == nullptr) { return; } - this->catalog_service->write_to_disk_on_blendfile_save(main->name); + if (save_catalogs_when_file_is_saved) { + this->catalog_service->write_to_disk(main->name); + } } void AssetLibrary::refresh_catalog_simplename(struct AssetMetaData *asset_data) @@ -166,15 +177,12 @@ void AssetLibrary::refresh_catalog_simplename(struct AssetMetaData *asset_data) asset_data->catalog_simple_name[0] = '\0'; return; } - 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. */ return; } - STRNCPY(asset_data->catalog_simple_name, catalog->simple_name.c_str()); } - } // namespace blender::bke diff --git a/source/blender/blenkernel/intern/asset_library_service.cc b/source/blender/blenkernel/intern/asset_library_service.cc index c5447de645b..aeded0bc128 100644 --- a/source/blender/blenkernel/intern/asset_library_service.cc +++ b/source/blender/blenkernel/intern/asset_library_service.cc @@ -83,7 +83,7 @@ AssetLibrary *AssetLibraryService::get_asset_library_on_disk(StringRefNull top_l AssetLibraryPtr lib_uptr = std::make_unique<AssetLibrary>(); AssetLibrary *lib = lib_uptr.get(); - lib->on_save_handler_register(); + lib->on_blend_save_handler_register(); lib->load(top_dir_trailing_slash); on_disk_libraries_.add_new(top_dir_trailing_slash, std::move(lib_uptr)); @@ -99,7 +99,7 @@ AssetLibrary *AssetLibraryService::get_asset_library_current_file() else { CLOG_INFO(&LOG, 2, "get current file lib (loaded)"); current_file_library_ = std::make_unique<AssetLibrary>(); - current_file_library_->on_save_handler_register(); + current_file_library_->on_blend_save_handler_register(); } AssetLibrary *lib = current_file_library_.get(); @@ -148,4 +148,19 @@ void AssetLibraryService::app_handler_unregister() on_load_callback_store_.arg = nullptr; } +bool AssetLibraryService::has_any_unsaved_catalogs() const +{ + if (current_file_library_ && current_file_library_->catalog_service->has_unsaved_changes()) { + return true; + } + + for (const auto &asset_lib_uptr : on_disk_libraries_.values()) { + if (asset_lib_uptr->catalog_service->has_unsaved_changes()) { + return true; + } + } + + return false; +} + } // namespace blender::bke diff --git a/source/blender/blenkernel/intern/asset_library_service.hh b/source/blender/blenkernel/intern/asset_library_service.hh index 63ffe56ab74..03df706bc42 100644 --- a/source/blender/blenkernel/intern/asset_library_service.hh +++ b/source/blender/blenkernel/intern/asset_library_service.hh @@ -66,6 +66,9 @@ class AssetLibraryService { /** Get the "Current File" asset library. */ AssetLibrary *get_asset_library_current_file(); + /** Returns whether there are any known asset libraries with unsaved catalog edits. */ + bool has_any_unsaved_catalogs() const; + protected: static std::unique_ptr<AssetLibraryService> instance_; diff --git a/source/blender/blenkernel/intern/asset_library_service_test.cc b/source/blender/blenkernel/intern/asset_library_service_test.cc index 36baa877454..ed132d7a8d8 100644 --- a/source/blender/blenkernel/intern/asset_library_service_test.cc +++ b/source/blender/blenkernel/intern/asset_library_service_test.cc @@ -22,6 +22,8 @@ #include "BLI_fileops.h" #include "BLI_path_util.h" +#include "BKE_appdir.h" + #include "CLG_log.h" #include "testing/testing.h" @@ -31,6 +33,7 @@ namespace blender::bke::tests { class AssetLibraryServiceTest : public testing::Test { public: CatalogFilePath asset_library_root_; + CatalogFilePath temp_library_path_; static void SetUpTestSuite() { @@ -48,11 +51,34 @@ class AssetLibraryServiceTest : public testing::Test { FAIL(); } asset_library_root_ = test_files_dir + "/" + "asset_library"; + temp_library_path_ = ""; } void TearDown() override { AssetLibraryService::destroy(); + + if (!temp_library_path_.empty()) { + BLI_delete(temp_library_path_.c_str(), true, true); + temp_library_path_ = ""; + } + } + + /* Register a temporary path, which will be removed at the end of the test. + * The returned path ends in a slash. */ + CatalogFilePath use_temp_path() + { + BKE_tempdir_init(""); + const CatalogFilePath tempdir = BKE_tempdir_session(); + temp_library_path_ = tempdir + "test-temporary-path/"; + return temp_library_path_; + } + + CatalogFilePath create_temp_path() + { + CatalogFilePath path = use_temp_path(); + BLI_dir_create_recursive(path.c_str()); + return path; } }; @@ -122,4 +148,52 @@ TEST_F(AssetLibraryServiceTest, catalogs_loaded) << "Catalogs should be loaded after getting an asset library from disk."; } +TEST_F(AssetLibraryServiceTest, has_any_unsaved_catalogs) +{ + AssetLibraryService *const service = AssetLibraryService::get(); + EXPECT_FALSE(service->has_any_unsaved_catalogs()) + << "Empty AssetLibraryService should have no unsaved catalogs"; + + AssetLibrary *const lib = service->get_asset_library_on_disk(asset_library_root_); + AssetCatalogService *const cat_service = lib->catalog_service.get(); + 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); + EXPECT_FALSE(service->has_any_unsaved_catalogs()) + << "Deletion of catalogs via AssetCatalogService should not tag as 'unsaved changes'."; + + cat_service->tag_has_unsaved_changes(); + EXPECT_TRUE(service->has_any_unsaved_catalogs()) + << "Tagging as having unsaved changes of a single catalog service should result in unsaved " + "changes being reported."; +} + +TEST_F(AssetLibraryServiceTest, has_any_unsaved_catalogs_after_write) +{ + const CatalogFilePath writable_dir = create_temp_path(); /* Has trailing slash. */ + const CatalogFilePath original_cdf_file = asset_library_root_ + "/blender_assets.cats.txt"; + CatalogFilePath writable_cdf_file = writable_dir + AssetCatalogService::DEFAULT_CATALOG_FILENAME; + BLI_path_slash_native(writable_cdf_file.data()); + ASSERT_EQ(0, BLI_copy(original_cdf_file.c_str(), writable_cdf_file.c_str())); + + AssetLibraryService *const service = AssetLibraryService::get(); + AssetLibrary *const lib = service->get_asset_library_on_disk(writable_dir); + + EXPECT_FALSE(service->has_any_unsaved_catalogs()) + << "Unchanged AssetLibrary should have no unsaved catalogs"; + + AssetCatalogService *const cat_service = lib->catalog_service.get(); + cat_service->tag_has_unsaved_changes(); + + EXPECT_TRUE(service->has_any_unsaved_catalogs()) + << "Tagging as having unsaved changes of a single catalog service should result in unsaved " + "changes being reported."; + + cat_service->write_to_disk(writable_dir + "dummy_path.blend"); + EXPECT_FALSE(service->has_any_unsaved_catalogs()) + << "Written AssetCatalogService should have no unsaved catalogs"; +} + } // namespace blender::bke::tests diff --git a/source/blender/blenkernel/intern/asset_library_test.cc b/source/blender/blenkernel/intern/asset_library_test.cc index 7c3587c6dfa..c6c949a7ec4 100644 --- a/source/blender/blenkernel/intern/asset_library_test.cc +++ b/source/blender/blenkernel/intern/asset_library_test.cc @@ -29,7 +29,7 @@ namespace blender::bke::tests { -class AssetLibraryServiceTest : public testing::Test { +class AssetLibraryTest : public testing::Test { public: static void SetUpTestSuite() { @@ -46,7 +46,7 @@ class AssetLibraryServiceTest : public testing::Test { } }; -TEST_F(AssetLibraryServiceTest, bke_asset_library_load) +TEST_F(AssetLibraryTest, bke_asset_library_load) { const std::string test_files_dir = blender::tests::flags_test_asset_dir(); if (test_files_dir.empty()) { @@ -73,7 +73,7 @@ TEST_F(AssetLibraryServiceTest, bke_asset_library_load) EXPECT_EQ("character/Ellie/poselib", poses_ellie->path.str()); } -TEST_F(AssetLibraryServiceTest, load_nonexistent_directory) +TEST_F(AssetLibraryTest, load_nonexistent_directory) { const std::string test_files_dir = blender::tests::flags_test_asset_dir(); if (test_files_dir.empty()) { |