blender
/
blender
134
400
Fork 583
Code Issues 6.3k Pull Requests 759 Projects 19 Wiki Activity

IO: Add support for multiple drag-n-drop files #107230

Merged
Brecht Van Lommel merged 20 commits from guishe/blender:dragndrop-files into main 2023-12-12 18:46:22 +01:00
Conversation 9 Commits 20 Files Changed 16 +76 -14
4 changed files with 76 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit de25bca8ad - Show all commits

18
source/blender/windowmanager/WM_api.hh
View File

@ -1426,12 +1426,26 @@ const char *WM_drag_get_item_name(wmDrag *drag);
* may be destructed.
*/
wmDragPath *WM_drag_create_path_data(blender::Span<const char *> paths);
/* If #drag contains path data, returns the first path int he path list. */
const char *WM_drag_get_single_path(const wmDrag *drag);
guishe marked this conversation as resolved Outdated
Julian Eisel commented 2023-11-22 13:06:45 +01:00
Outdated
Review
Copy Link

This is returned by value, so const for the return type doesn't make sense (some static analyzers will warn I think).

This is returned by value, so `const` for the return type doesn't make sense (some static analyzers will warn I think).
/* If #drag contains path data, returns the first path in the path list that maches a
* a `file_type`.*/
/*
guishe marked this conversation as resolved Outdated
Julian Eisel commented 2023-11-22 12:44:52 +01:00
Outdated
Review
Copy Link

The "For internal use only" part doesn't make sense to me, this is a public API.

The "For internal use only" part doesn't make sense to me, this is a public API.
* \param drag: The drag that could contain drag path data.
* \param file_type: `eFileSel_File_Types` bit flag
*/
const char *WM_drag_get_single_path(const wmDrag *drag, int file_type);
blender::Span<std::string> WM_drag_get_paths(const wmDrag *drag);
/* If #drag contains path data, returns if any file path match a `file_type`.*/
/*
* \param drag: The drag that could contain drag path data.
* \param file_type: `eFileSel_File_Types` bit flag
*/
bool WM_drag_has_path_file_type(const wmDrag *drag, int file_type);
/**
* Note that even though the enum return type uses bit-flags, this should never have multiple
* type-bits set, so `ELEM()` like comparison is possible. Only indicates the file type of first
* path in `wmDragPath.paths`.
* type-bits set, so `ELEM()` like comparison is possible. To check all paths or to do a bit-flag
* check use `WM_drag_has_path_file_type(drag,file_type)` instead.
*/
int /* eFileSel_File_Types */ WM_drag_get_path_file_type(const wmDrag *drag);

8
source/blender/windowmanager/WM_types.hh
View File

@ -1174,10 +1174,10 @@ struct wmDragAssetListItem {
struct wmDragPath {
blender::Vector<std::string> paths;
/* Note that even though the enum type uses bit-flags, this should never have multiple type-bits
* set, so `ELEM()` like comparison is possible. Only indicates the file type of first path in
* `wmDragPath.paths`. */
int file_type; /* eFileSel_File_Types */
/* File type of each path in #paths. */
blender::Vector<int> file_types; /* eFileSel_File_Types */
/* Bit flag of file types in #paths. */
int file_types_bit_flag; /* eFileSel_File_Types */
std::string tooltip;
};

35
source/blender/windowmanager/intern/wm_dragdrop.cc
View File

@ -768,10 +768,10 @@ wmDragPath *WM_drag_create_path_data(blender::Span<const char *> paths)
BLI_assert(!paths.is_empty());
brecht marked this conversation as resolved Outdated
Bastien Montagne commented 2023-11-07 16:11:29 +01:00
Outdated
Review
Copy Link

Is there a reason for using string extension instead of the (already computed) file_type here (and then comparing to the result of ED_path_extension_type for all other paths in the loop below)?

If yes, should be documented in a comment.

Current approach would also 'fail' (filter out valid paths) e.g. in case of paths to a mix of JPG and PNG images. Or paths containing several USD files with different extensions.

Is there a reason for using string extension instead of the (already computed) `file_type` here (and then comparing to the result of `ED_path_extension_type` for all other paths in the loop below)? If yes, should be documented in a comment. Current approach would also 'fail' (filter out valid paths) e.g. in case of paths to a mix of JPG and PNG images. Or paths containing several USD files with different extensions.
Brecht Van Lommel commented 2023-11-07 16:43:08 +01:00
Outdated
Review
Copy Link

ED_path_extension_type is only for file types that Blender natively supports, while this system is meant to work for arbitrary add-ons.

`ED_path_extension_type` is only for file types that Blender natively supports, while this system is meant to work for arbitrary add-ons.
Guillermo Venegas commented 2023-11-07 19:39:42 +01:00
Outdated
Review
Copy Link

Initially I had the idea of just the one extension for file handler, but since file handlers could have a list of files they can take, here we can copy all and then see which ones are useful for the file handlers.

path_data->file_type would only be used internally with the first file, and since these changes don't change the behavior I think it's safe to keep it and collect all the file paths, I'll leave a note that this file_type will just you describe the first path.

Initially I had the idea of just the one extension for file handler, but since file handlers could have a list of files they can take, here we can copy all and then see which ones are useful for the file handlers. `path_data->file_type` would only be used internally with the first file, and since these changes don't change the behavior I think it's safe to keep it and collect all the file paths, I'll leave a note that this `file_type` will just you describe the first path.
Bastien Montagne commented 2023-11-08 18:27:50 +01:00
Outdated
Review
Copy Link

Ah yes, makes sense, better not do any filtering here then indeed.

Ah yes, makes sense, better not do any filtering here then indeed.
wmDragPath *path_data = MEM_new<wmDragPath>("wmDragPath");
guishe marked this conversation as resolved Outdated
Julian Eisel commented 2023-11-22 12:28:59 +01:00
Outdated
Review
Copy Link

This is just const char* isn't it? In that case auto just hides type information with no real benefit, prefer not using it in such cases.

This is just `const char*` isn't it? In that case `auto` just hides type information with no real benefit, prefer not using it in such cases.
path_data->file_type = ED_path_extension_type(paths[0]);
for (const char *path : paths) {
path_data->paths.append(path);
path_data->file_types_bit_flag |= ED_path_extension_type(path);
path_data->file_types.append(ED_path_extension_type(path));
}
path_data->tooltip = path_data->paths[0];
guishe marked this conversation as resolved Outdated
Bastien Montagne commented 2023-11-07 16:32:22 +01:00
Outdated
Review
Copy Link

There is no reason to use C-strings here. std::string and fmt::format (extern library we are using since we are not yet on C++20) should be even easier to use.

There is no reason to use C-strings here. `std::string` and `fmt::format` (extern library we are using since we are not yet on C++20) should be even easier to use.
Bastien Montagne commented 2023-11-08 18:24:09 +01:00
Review
Copy Link

No final point in our UI strings: "Dragging {} files"

No final point in our UI strings: `"Dragging {} files"`
@ -800,6 +800,35 @@ const char *WM_drag_get_single_path(const wmDrag *drag)
return path_data->paths[0].c_str();
guishe marked this conversation as resolved Outdated
Julian Eisel commented 2023-11-22 13:07:05 +01:00
Outdated
Review
Copy Link

Same, remove const from return type.

Same, remove `const` from return type.
}
const char *WM_drag_get_single_path(const wmDrag *drag, int file_type)
guishe marked this conversation as resolved Outdated
Bastien Montagne commented 2023-11-07 16:34:05 +01:00
Outdated
Review
Copy Link

return nullptr;

`return nullptr;`
{
if (drag->type != WM_DRAG_PATH) {
return nullptr;
}
const wmDragPath *path_data = static_cast<const wmDragPath *>(drag->poin);
auto const file_types = path_data->file_types;
auto itr = std::find_if(
file_types.begin(), file_types.end(), [file_type](const int file_fype_test) {
return file_fype_test & file_type;
});
if (itr == file_types.end()) {
return nullptr;
}
const int index = itr - file_types.begin();
return path_data->paths[index].c_str();
}
bool WM_drag_has_path_file_type(const wmDrag *drag, int file_type)
{
if (drag->type != WM_DRAG_PATH) {
return false;
}
const wmDragPath *path_data = static_cast<const wmDragPath *>(drag->poin);
return bool(path_data->file_types_bit_flag & file_type);
}
blender::Span<std::string> WM_drag_get_paths(const wmDrag *drag)
{
if (drag->type != WM_DRAG_PATH) {
@ -817,7 +846,7 @@ int WM_drag_get_path_file_type(const wmDrag *drag)
}
const wmDragPath *path_data = static_cast<const wmDragPath *>(drag->poin);
return path_data->file_type;
return path_data->file_types[0];
}
/* ************** draw ***************** */

29
source/blender/windowmanager/intern/wm_dragdrop_test.cc
View File

@ -4,9 +4,6 @@
#include "testing/testing.h"
/* #bContextStore. */
#include "BKE_context.h"
/* #eFileSel_File_Types. */
#include "DNA_space_types.h"
brecht marked this conversation as resolved
Julian Eisel commented 2023-11-22 12:36:44 +01:00
Review
Copy Link

Doesn't seem to be needed.

Doesn't seem to be needed.
Guillermo Venegas commented 2023-11-24 15:53:06 +01:00
Review
Copy Link

Needed by wmDrag.drop_state.ui_context, is a std::unique_ptr<bContextStore>

Needed by `wmDrag.drop_state.ui_context`, is a `std::unique_ptr<bContextStore>`
Julian Eisel commented 2023-11-28 15:29:00 +01:00
Review
Copy Link

While not a big deal, this include shouldn't be necessary. Committed e3b3399bcb so this can be removed now.

While not a big deal, this include shouldn't be necessary. Committed e3b3399bcb so this can be removed now.
@ -38,7 +35,11 @@ TEST(wm_drag, wmDragPath)
EXPECT_STREQ(WM_drag_get_single_path(&drag), "text_file.txt");
EXPECT_EQ(WM_drag_get_path_file_type(&drag), FILE_TYPE_TEXT);
EXPECT_EQ(WM_drag_get_paths(&drag), expected_file_paths.as_span());
EXPECT_STREQ(WM_drag_get_single_path(&drag, FILE_TYPE_TEXT), "text_file.txt");
EXPECT_EQ(WM_drag_get_single_path(&drag, FILE_TYPE_BLENDER), nullptr);
EXPECT_TRUE(
WM_drag_has_path_file_type(&drag, FILE_TYPE_BLENDER | FILE_TYPE_TEXT | FILE_TYPE_IMAGE));
EXPECT_FALSE(WM_drag_has_path_file_type(&drag, FILE_TYPE_BLENDER | FILE_TYPE_IMAGE));
MEM_delete(path_data);
}
{
@ -58,7 +59,20 @@ TEST(wm_drag, wmDragPath)
EXPECT_STREQ(WM_drag_get_single_path(&drag), "blender.blend");
EXPECT_EQ(WM_drag_get_path_file_type(&drag), FILE_TYPE_BLENDER);
EXPECT_EQ(WM_drag_get_paths(&drag), expected_file_paths.as_span());
EXPECT_STREQ(WM_drag_get_single_path(&drag, FILE_TYPE_BLENDER), "blender.blend");
EXPECT_STREQ(WM_drag_get_single_path(&drag, FILE_TYPE_IMAGE), "image.png");
EXPECT_STREQ(WM_drag_get_single_path(&drag, FILE_TYPE_TEXT), "text_file.txt");
EXPECT_STREQ(
WM_drag_get_single_path(&drag, FILE_TYPE_BLENDER | FILE_TYPE_TEXT | FILE_TYPE_IMAGE),
"blender.blend");
EXPECT_STREQ(WM_drag_get_single_path(&drag, FILE_TYPE_TEXT | FILE_TYPE_IMAGE),
"text_file.txt");
EXPECT_EQ(WM_drag_get_single_path(&drag, FILE_TYPE_ASSET), nullptr);
EXPECT_TRUE(
WM_drag_has_path_file_type(&drag, FILE_TYPE_BLENDER | FILE_TYPE_TEXT | FILE_TYPE_IMAGE));
EXPECT_TRUE(WM_drag_has_path_file_type(&drag, FILE_TYPE_BLENDER | FILE_TYPE_IMAGE));
EXPECT_TRUE(WM_drag_has_path_file_type(&drag, FILE_TYPE_IMAGE));
EXPECT_FALSE(WM_drag_has_path_file_type(&drag, FILE_TYPE_ASSET));
MEM_delete(path_data);
}
{
@ -68,6 +82,11 @@ TEST(wm_drag, wmDragPath)
EXPECT_EQ(WM_drag_get_single_path(&drag), nullptr);
EXPECT_EQ(WM_drag_get_path_file_type(&drag), 0);
EXPECT_EQ(WM_drag_get_paths(&drag).size(), 0);
EXPECT_EQ(WM_drag_get_single_path(
&drag, FILE_TYPE_BLENDER | FILE_TYPE_IMAGE | FILE_TYPE_TEXT | FILE_TYPE_ASSET),
nullptr);
EXPECT_FALSE(WM_drag_has_path_file_type(
&drag, FILE_TYPE_BLENDER | FILE_TYPE_IMAGE | FILE_TYPE_TEXT | FILE_TYPE_ASSET));
}
}