Fix #115192: Inconsistent behavior renaming data-blocks #116246

Pratik Borhade · 2023-12-16T06:19:57+01:00

Pratik Borhade commented

2023-12-16 06:19:57 +01:00

This is due to BLI_findstring returning wrong id from passed name.
Two IDs can have same name before making them unique so it can return
wrong id. To fix this, pass id argument instead of id_name to
BLI_libblock_ensure_unique_name and skip the use of BLI_findstring

This is due to `BLI_findstring` returning wrong id from passed name. Two IDs can have same name before making them unique so it can return wrong id. To fix this, pass id argument instead of id_name to `BLI_libblock_ensure_unique_name` and skip the use of `BLI_findstring`

👍 1

Pratik Borhade added 1 commit 2023-12-16 06:20:07 +01:00

05810e0879 Fix #115192 : Inconsistancy renaming datablock behaviour

This is due to `BLI_findstring` returning wrong id from passed name.
Two IDs can have same name before making them unique so it can return
wrong id. To fix this, pass id argument instead of id_name to
`BLI_libblock_ensure_unique_name` and skip the use of `BLI_findstring`

Pratik Borhade requested review from Brecht Van Lommel 2023-12-16 06:20:24 +01:00

Pratik Borhade requested review from Bastien Montagne 2023-12-16 06:20:24 +01:00

Pratik Borhade added the

Module

User Interface

label 2023-12-16 06:20:41 +01:00

Brecht Van Lommel requested changes 2023-12-18 15:03:45 +01:00

Brecht Van Lommel left a comment

A difference is that is_memfile_undo_written will now be set regardless if the name is actually changed.

I think we can now check the return value of BKE_id_new_name_validate for that, same as BKE_libblock_rename does just below.

However looking at the implementation of BKE_id_new_name_validate, it doesn't seem to account for name changes to due e.g. invalid characters. Which I think could be fixed as follows:

diff --git a/source/blender/blenkernel/intern/lib_id.cc b/source/blender/blenkernel/intern/lib_id.cc
index 2b7e57e..5956ebe 100644
--- a/source/blender/blenkernel/intern/lib_id.cc
+++ b/source/blender/blenkernel/intern/lib_id.cc
@@ -1632,6 +1632,7 @@ bool BKE_id_new_name_validate(
   }
 
   result = BKE_main_namemap_get_name(bmain, id, name, false);
+  result |= !STREQ(id->name + 2, name);
 
   BLI_strncpy(id->name + 2, name, sizeof(id->name) - 2);
   id_sort_by_name(lb, id, nullptr);

A difference is that `is_memfile_undo_written` will now be set regardless if the name is actually changed. I think we can now check the return value of `BKE_id_new_name_validate` for that, same as `BKE_libblock_rename` does just below. However looking at the implementation of `BKE_id_new_name_validate`, it doesn't seem to account for name changes to due e.g. invalid characters. Which I think could be fixed as follows: ``` diff --git a/source/blender/blenkernel/intern/lib_id.cc b/source/blender/blenkernel/intern/lib_id.cc index 2b7e57e..5956ebe 100644 --- a/source/blender/blenkernel/intern/lib_id.cc +++ b/source/blender/blenkernel/intern/lib_id.cc @@ -1632,6 +1632,7 @@ bool BKE_id_new_name_validate( } result = BKE_main_namemap_get_name(bmain, id, name, false); + result |= !STREQ(id->name + 2, name); BLI_strncpy(id->name + 2, name, sizeof(id->name) - 2); id_sort_by_name(lb, id, nullptr); ```

👍 1

Pratik Borhade added 1 commit 2023-12-20 12:50:41 +01:00

1281e7cb1e Change is_memfile_undo_written when suffix added to id name

Brecht Van Lommel approved these changes 2023-12-20 13:26:13 +01:00

Brecht Van Lommel left a comment

Looks good to me, but since @mont29 is owner of this code would wait for review from him too.

Bastien Montagne commented

2023-12-20 17:04:14 +01:00

@brecht as noticed in the report (#115192), this was already reported long time ago as #71244.

Back then, there were some unclear/inconclusive discussions about whether we always want to modify the 'current' ID's name, or whether in some cases (e.g. when renaming from the UI), we'd rather change the other ID's name.

I would rather have the consistent behavior proposed by this PR as a starting point, and then evaluate whether we really need some tweaks to it (or additional operators) later, but would love to hear @ideasman42 here as well?

@brecht as noticed in the report (#115192), this was already reported long time ago as #71244. Back then, there were some [unclear/inconclusive discussions](https://projects.blender.org/blender/blender/issues/71244#issuecomment-383129) about whether we always want to modify the 'current' ID's name, or whether in some cases (e.g. when renaming from the UI), we'd rather change the other ID's name. I would rather have the consistent behavior proposed by this PR as a starting point, and then evaluate whether we really need some tweaks to it (or additional operators) later, but would love to hear @ideasman42 here as well?

Brecht Van Lommel commented

2023-12-21 19:32:41 +01:00

I think it makes sense to allow taking the name of other datablocks from the UI. But this should then have a confirmation popup, or at least a warning in the status bar so the user can undo it. Both for individual and batch renames.

Campbell Barton commented

2024-01-15 03:14:49 +01:00

This seems much better than the current behavior (how it should have worked all along).

The only other comment is the doc-strings in this area should be improved/clarified however that can be done as part of a separate change as they're not made invalid by this commit.

This seems much better than the current behavior (how it should have worked all along). The only other comment is the doc-strings in this area should be improved/clarified however that can be done as part of a separate change as they're not made invalid by this commit.

Campbell Barton approved these changes 2024-01-15 03:15:04 +01:00

Campbell Barton changed title from ~~Fix #115192: Inconsistancy renaming datablock behaviour~~ to Fix #115192: Inconsist behavior renaming data-blocks

2024-01-15 03:24:34 +01:00

Campbell Barton changed title from ~~Fix #115192: Inconsist behavior renaming data-blocks~~ to Fix #115192: Inconsistent behavior renaming data-blocks

2024-01-15 03:25:10 +01:00

Bastien Montagne approved these changes 2024-01-15 12:41:57 +01:00

Bastien Montagne left a comment

There is also a terrible name prefix usage here for BLI_libblock_ensure_unique_name (should be BKE_ prefix, comes from original cleanup renames 641d4e2b7f). ;)

Will fix as soon as this PR is merged.

There is also a terrible name prefix usage here for `BLI_libblock_ensure_unique_name` (should be `BKE_ prefix`, comes from original cleanup renames 641d4e2b7fc4fd). ;) Will fix as soon as this PR is merged.

Pratik Borhade merged commit 195bb4f8f5 into main

2024-01-15 12:45:57 +01:00

Pratik Borhade deleted branch 115192-id-naming

2024-01-15 12:46:00 +01:00

Pratik Borhade referenced this issue from a commit

2024-01-15 12:46:02 +01:00

Fix #115192: Inconsistent behavior renaming data-blocks

Pratik Borhade commented

2024-01-15 12:46:26 +01:00

Merged, thanks for reviewing :)

🎉 1

Bastien Montagne referenced this pull request