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
4 changed files with 94 additions and 3 deletions
Showing only changes of commit b5770d6b3b - Show all commits

View File

@ -210,3 +210,11 @@ blender_add_lib_nolist(bf_windowmanager "${SRC}" "${INC}" "${INC_SYS}" "${LIB}")
# RNA_prototypes.h
add_dependencies(bf_windowmanager bf_rna)
if(WITH_GTESTS)
set(TEST_SRC
intern/wm_dragdrop_test.cc
)
include(GTestTesting)
blender_add_test_lib(bf_wm_tests "${TEST_SRC}" "${INC};${TEST_INC}" "${INC_SYS}" "${LIB}")
endif()

View File

@ -762,6 +762,7 @@ const ListBase *WM_drag_asset_list_get(const wmDrag *drag)
wmDragPath *WM_drag_create_path_data(blender::Span<const char *> paths)
{
BLI_assert(!paths.is_empty());
wmDragPath *path_data = MEM_new<wmDragPath>("wmDragPath");
path_data->file_type = ED_path_extension_type(paths[0]);
brecht marked this conversation as resolved Outdated

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.

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.

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.

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

Ah yes, makes sense, better not do any filtering here then indeed.
@ -774,7 +775,7 @@ wmDragPath *WM_drag_create_path_data(blender::Span<const char *> paths)
if (path_data->paths.size() > 1) {
std::string path_count = std::to_string(path_data->paths.size());
guishe marked this conversation as resolved Outdated

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.
Review

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

No final point in our UI strings: `"Dragging {} files"`
path_data->tooltip = fmt::format(TIP_("Dragging {} files."), path_count);
path_data->tooltip = fmt::format(TIP_("Dragging {} files"), path_count);
}
return path_data;

View File

@ -0,0 +1,74 @@
/* SPDX-FileCopyrightText: 2023 Blender Authors
*
* SPDX-License-Identifier: Apache-2.0 */
#include "testing/testing.h"
/* #bContextStore. */
#include "BKE_context.h"
brecht marked this conversation as resolved
Review

Doesn't seem to be needed.

Doesn't seem to be needed.

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>`
Review

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.
/* #eFileSel_File_Types. */
#include "DNA_space_types.h"
#include "WM_api.hh"
#include "WM_types.hh"
namespace blender::tests {
TEST(wm_dragdrop, create)
guishe marked this conversation as resolved Outdated

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)`.
{
{
/**
* NOTE: `WM_drag_create_path_data` gets the `file_type` from the first path in `paths` and
* only needs its extension, so there is no need to describe a full path here that can have a
* different format on Windows or Linux. However callers must ensure that they are valid paths.
*/
blender::Vector<const char *> paths{"text_file.txt"};
wmDragPath *path_data = WM_drag_create_path_data(paths);
blender::Vector<std::string> expected_file_paths{"text_file.txt"};
EXPECT_EQ(path_data->paths.size(), 1);
EXPECT_EQ(path_data->tooltip, "text_file.txt");
EXPECT_EQ(path_data->paths, expected_file_paths);
/** Test `wmDrag` path data getters. */
wmDrag drag;
drag.type = WM_DRAG_PATH;
drag.poin = path_data;
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());
MEM_delete(path_data);
}
{
blender::Vector<const char *> paths = {"blender.blend", "text_file.txt", "image.png"};
wmDragPath *path_data = WM_drag_create_path_data(paths);
blender::Vector<std::string> expected_file_paths = {
"blender.blend", "text_file.txt", "image.png"};
EXPECT_EQ(path_data->paths.size(), 3);
EXPECT_EQ(path_data->tooltip, "Dragging 3 files");
EXPECT_EQ(path_data->paths, expected_file_paths);
/** Test `wmDrag` path data getters. */
wmDrag drag;
drag.type = WM_DRAG_PATH;
drag.poin = path_data;
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());
MEM_delete(path_data);
}
{
/** Test `wmDrag` path data getters when the drag type is different to `WM_DRAG_PATH`. */
wmDrag drag;
drag.type = WM_DRAG_COLOR;
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);
}
}
} // namespace blender::tests

View File

@ -15,6 +15,8 @@
#include <cstring>
#include <thread>
#include "CLG_log.h"
#include "DNA_listBase.h"
#include "DNA_screen_types.h"
#include "DNA_windowmanager_types.h"
@ -63,6 +65,8 @@
#include "ED_scene.hh"
#include "ED_screen.hh"
#include <fmt/format.h>
guishe marked this conversation as resolved Outdated

Doesn't seem to be needed.

Doesn't seem to be needed.
#include "IMB_imbuf.h"
#include "IMB_imbuf_types.h"
@ -1569,13 +1573,17 @@ 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) {
/* try to get icon type from extension */
CLOG_INFO(WM_LOG_EVENTS, 1, "Drop %d files:", stra->count);
for (const char *path : blender::Span((char **)stra->strings, stra->count)) {
CLOG_INFO(WM_LOG_EVENTS, 1, "%s", path);
}
/* Try to get icon type from extension of the first path. */
int icon = ED_file_extension_icon((char *)stra->strings[0]);
wmDragPath *path_data = WM_drag_create_path_data(
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; /* only one drop element supported now */
break;
guishe marked this conversation as resolved Outdated

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

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