blender
/
blender
135
400
Fork 583
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 +8 -11
5 changed files with 8 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 21a07782fb - Show all commits

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

@ -1427,11 +1427,11 @@ const char *WM_drag_get_item_name(wmDrag *drag);
*/
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);
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. For internal use only, and only
* indicates the file type of first path in `wmDragPath.paths`.
* type-bits set, so `ELEM()` like comparison is possible. Only indicates the file type of first
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.
* path in `wmDragPath.paths`.
*/
int /* eFileSel_File_Types */ WM_drag_get_path_file_type(const wmDrag *drag);

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

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

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

@ -767,7 +767,7 @@ 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.
for (auto path : paths) {
for (const char *path : paths) {
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->paths.append(path);
}
@ -797,7 +797,7 @@ const char *WM_drag_get_single_path(const wmDrag *drag)
return path_data->paths[0].c_str();
}
const blender::Span<std::string> WM_drag_get_paths(const wmDrag *drag)
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:07:05 +01:00
Outdated
Review
Copy Link

Same, remove const from return type.

Same, remove `const` from return type.
{
if (drag->type != WM_DRAG_PATH) {
return blender::Span<std::string>();
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;`

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

@ -15,7 +15,7 @@
namespace blender::tests {
TEST(wm_dragdrop, create)
TEST(wm_drag, wmDragPath)
guishe marked this conversation as resolved Outdated
Julian Eisel commented 2023-11-22 12:40:21 +01:00
Outdated
Review
Copy Link

Should mention that this is testing for paths specifically. Suggest something like TEST(wm_drag, wmDragPath).

Should mention that this is testing for paths specifically. Suggest something like `TEST(wm_drag, wmDragPath)`.
{
{
/**

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

@ -65,8 +65,6 @@
#include "ED_scene.hh"
#include "ED_screen.hh"
#include <fmt/format.h>
#include "IMB_imbuf.h"
guishe marked this conversation as resolved Outdated
Julian Eisel commented 2023-11-22 12:42:56 +01:00
Outdated
Review
Copy Link

Doesn't seem to be needed.

Doesn't seem to be needed.
#include "IMB_imbuf_types.h"
@ -1583,7 +1581,6 @@ static bool ghost_event_proc(GHOST_EventHandle evt, GHOST_TUserDataPtr C_void_pt
blender::Span((char **)stra->strings, stra->count));
WM_event_start_drag(C, icon, WM_DRAG_PATH, path_data, 0.0, WM_DRAG_NOP);
/* Void pointer should point to string, it makes a copy. */
break;
}
}
guishe marked this conversation as resolved Outdated
Julian Eisel commented 2023-11-22 12:31:02 +01:00
Outdated
Review
Copy Link

Redundant break since there's a break a few lines below.

Redundant `break` since there's a break a few lines below.