blender
/
blender
134
399
Fork 582
Code Issues 6.3k Pull Requests 758 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 +13 -23
4 changed files with 13 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0b8d3d77dc - Show all commits

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

@ -1423,14 +1423,15 @@ const char *WM_drag_get_item_name(wmDrag *drag);
/* Paths drag and drop. */
/**
* \param paths: The paths to drag. Values will be copied into the drag data so the passed strings
* may be destructed. Only paths that share the same extension of the first file will be copied.
* may be destructed.
*/
wmDragPath *WM_drag_create_path_data(blender::Span<const char *> paths);
const char *WM_drag_get_single_path(const wmDrag *drag);
const blender::Span<std::string> WM_drag_get_paths(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).
/**
* 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.
* type-bits set, so `ELEM()` like comparison is possible. For internal use only, and only
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.
* indicates the file type of first path in `wmDragPath.paths`.
*/
int /* eFileSel_File_Types */ WM_drag_get_path_file_type(const wmDrag *drag);

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

@ -1175,7 +1175,8 @@ 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. */
* set, so `ELEM()` like comparison is possible. For internal use only, and only indicates the file type
* of first path in `wmDragPath.paths`. */
int file_type; /* eFileSel_File_Types */
std::string tooltip;
};

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

@ -60,6 +60,7 @@
#include "wm_event_system.h"
#include "wm_window.hh"
#include <fmt/format.h>
/* ****************************************************** */
static ListBase dropboxes = {nullptr, nullptr};
@ -765,28 +766,17 @@ wmDragPath *WM_drag_create_path_data(blender::Span<const char *> paths)
path_data->file_type = ED_path_extension_type(paths[0]);
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.
const char *extension = BLI_path_extension(paths[0]);
for (auto path : paths) {
const char *test_ext = BLI_path_extension(path);
if (extension == test_ext || (extension && test_ext && STREQ(extension, test_ext))) {
path_data->paths.append(path);
}
path_data->paths.append(path);
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.
}
const char *tooltip = path_data->paths[0].c_str();
char tooltip_buffer[256];
path_data->tooltip = path_data->paths[0];
if (path_data->paths.size() > 1) {
BLI_snprintf(tooltip_buffer,
ARRAY_SIZE(tooltip_buffer),
TIP_("Dragging %d %s files."),
path_data->paths.size(),
extension ? extension : TIP_("Folder"));
tooltip = tooltip_buffer;
std::string path_count = std::to_string(path_data->paths.size());
path_data->tooltip = fmt::format(TIP_("Dragging {} files."), path_count);
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"`
}
path_data->tooltip = tooltip;
return path_data;
}
@ -800,7 +790,7 @@ static void wm_drag_free_path_data(wmDragPath **path_data)
const char *WM_drag_get_single_path(const wmDrag *drag)
{
if (drag->type != WM_DRAG_PATH) {
nullptr;
return nullptr;
}
const wmDragPath *path_data = static_cast<const wmDragPath *>(drag->poin);
@ -1071,9 +1061,8 @@ void wm_drags_draw(bContext *C, wmWindow *win)
wmWindowViewport(win);
}
/* Drawing should be allowed to assume the context from
* handling and polling (that's why we restore it
* above). */
/* Drawing should be allowed to assume the context from handling and polling (that's why we
restore it above). */
if (drag->drop_state.active_dropbox->draw_droptip) {
drag->drop_state.active_dropbox->draw_droptip(C, win, drag, xy);
continue;

1
source/blender/windowmanager/intern/wm_window.cc
View File

@ -1569,7 +1569,6 @@ static bool ghost_event_proc(GHOST_EventHandle evt, GHOST_TUserDataPtr C_void_pt
const GHOST_TStringArray *stra = static_cast<const GHOST_TStringArray *>(ddd->data);
if (stra->count) {
printf("drop file %s\n", stra->strings[0]);
/* try to get icon type from extension */
guishe marked this conversation as resolved Outdated
Bastien Montagne commented 2023-11-07 16:37:50 +01:00
Outdated
Review
Copy Link

This printf can be removed imho, it's now not that useful.

If needed, proper logging with CLOG and a loop over all handled filepaths should be done instead.

This printf can be removed imho, it's now not that useful. If needed, proper logging with `CLOG` and a loop over all handled filepaths should be done instead.
Guillermo Venegas commented 2023-11-07 19:36:56 +01:00
Outdated
Review
Copy Link

Maybe it's better to show which files were discarded

Maybe it's better to show which files were discarded
Bastien Montagne commented 2023-11-08 18:21:25 +01:00
Outdated
Review
Copy Link

I would say both...

I would say both...
int icon = ED_file_extension_icon((char *)stra->strings[0]);
wmDragPath *path_data = WM_drag_create_path_data(