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

7 files changed, 151 insertions, 26 deletions

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()) {