Fix #109561: Overwrite existing imagepath based on relativepath property #109815

Pratik Borhade · 2023-07-07T12:18:09+02:00

Pratik Borhade commented

2023-07-07 12:18:09 +02:00

Update the filepath of an image when it already exists in the main
storage. This is useful when relative_path property is changed and we
attempt to open the same image file again.

Also remove is_relative_path parameter. Not required anymore because
range->filepath is correctly set to absolute/relative. This is also
passed in BKE_image_load_exists_ex to set path to non-exiting image.

Update the filepath of an image when it already exists in the main storage. This is useful when relative_path property is changed and we attempt to open the same image file again. Also remove `is_relative_path` parameter. Not required anymore because `range->filepath` is correctly set to absolute/relative. This is also passed in `BKE_image_load_exists_ex` to set path to non-exiting image.

Pratik Borhade added 1 commit 2023-07-07 12:18:23 +02:00

Fix #109561 : Overwrite existing imagepath based on relativepath property 29db26024b

Update the filepath of an image when it already exists in the main
storage. This is useful when relative_path property is changed and we
attempt to open the same image file again.

Pratik Borhade requested review from Pablo Vazquez 2023-07-07 12:18:39 +02:00

Pratik Borhade requested review from Jeroen Bakker 2023-07-07 12:18:40 +02:00

Pratik Borhade commented

2023-07-07 12:20:38 +02:00

Well this is actually a 2-3 lines of change
but Gitea is showing the diff in more complex way :)

Not sure if this change is really useful but submitted the PR anyway.

Well this is actually a 2-3 lines of change but Gitea is showing the diff in more complex way :) Not sure if this change is really useful but submitted the PR anyway.

Pablo Vazquez approved these changes 2023-07-22 18:51:04 +02:00

Pablo Vazquez left a comment

Accepting it from the usability point of view. Will leave code review to @Jeroen-Bakker .

Brecht Van Lommel requested changes 2023-08-15 19:44:08 +02:00

source/blender/editors/space_image/image_ops.c Outdated

						
				@ -1299,0 +1296,4 @@

				  if (exists) {

				    STRNCPY(ima->filepath, range->filepath);

				    return ima;

				  }

Brecht Van Lommel commented

2023-08-15 19:43:48 +02:00

Doesn't this always make the path absolute?

I would think this following code needs to run for both the exists and not exists case?

  if (is_relative_path) {
    BLI_path_rel(ima->filepath, relbase);
  }

Doesn't this always make the path absolute? I would think this following code needs to run for both the exists and not exists case? ``` if (is_relative_path) { BLI_path_rel(ima->filepath, relbase); } ```

Pratik Borhade commented

2023-08-22 07:56:06 +02:00

Doesn't this always make the path absolute?

No. Before calling image_open_single, range->filepath is set to relative/absolute in ED_image_filesel_detect_sequences function execution.

So I don't think additional change is needed :)

@brecht , let me know if you think this change is not good and the explanation is not convincing. I'll update the PR then 😅

> Doesn't this always make the path absolute? No. Before calling `image_open_single`, range->filepath is set to relative/absolute in [ED_image_filesel_detect_sequences](https://projects.blender.org/blender/blender/src/branch/main/source/blender/editors/space_image/image_sequence.cc#L164) function execution. So I don't think additional change is needed :) @brecht , let me know if you think this change is not good and the explanation is not convincing. I'll update the PR then 😅

Brecht Van Lommel commented

2023-08-30 11:30:05 +02:00

Then that would mean this code is also not needed for the !exists case, and can be removed? Otherwise it's confusing to me.

Then that would mean this code is also not needed for the `!exists` case, and can be removed? Otherwise it's confusing to me.

Pratik Borhade commented

2023-09-01 13:01:18 +02:00

Yes, not needed. removed :)

Brecht Van Lommel removed review request for Jeroen Bakker 2023-08-15 19:44:13 +02:00

Pratik Borhade added 1 commit 2023-08-22 11:26:47 +02:00

Merge branch 'main' into 109561-overrite-image-path 0ae0039006

Fixed merge conflicts

Brecht Van Lommel requested changes 2023-08-30 11:30:17 +02:00

Brecht Van Lommel left a comment

Requesting changes.

Pratik Borhade added 1 commit 2023-09-01 12:50:49 +02:00

Remove is_relative_path c50d6ea4c7

This is not needed. range->filepath is correctly set to absoulte/relative.
range->filepath is also passed in `BKE_image_load_exists_ex` to assign path
to non-exiting image.

Brecht Van Lommel approved these changes 2023-09-01 13:19:39 +02:00

Brecht Van Lommel left a comment

Isn't there a warning about is_relative_path being unused now? I think it can just be removed, and let other parts of the code handle that RNA property.

Isn't there a warning about `is_relative_path` being unused now? I think it can just be removed, and let other parts of the code handle that RNA property.

👍 1

Pratik Borhade added 2 commits 2023-09-01 13:40:21 +02:00

Merge branch 'main' into 109561-overrite-image-path 96c3d85a60

Remove unused variable c2e58e6951

Pratik Borhade commented

2023-09-01 13:41:27 +02:00

No warning but yes, it's not used anywhere else. Removed.

Brecht Van Lommel approved these changes 2023-09-01 13:47:38 +02:00

Pratik Borhade merged commit 1c0a1ae801 into main

2023-09-02 07:20:01 +02:00

Pratik Borhade deleted branch 109561-overrite-image-path

2023-09-02 07:20:03 +02:00