IO: Add support for multiple drag-n-drop files #107230
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#107230
Loading…
Reference in New Issue
No description provided.
Delete Branch "guishe/blender:dragndrop-files"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
There are operators in blender that allow the user to import multiple
files at the same time, however this functionality is only implemented
when importing with blender's file browser, drag and drop files only
takes first selected file.
The patch adds support for drag and drop multiple files.
Notes:
selected file.
so they will still import one.
Here is a video showing what this patch would allow along with this other patch #106871 (a change is needed to get all filepaths)
IO: Add support for multiple DragAndDrop filesto WIP: IO: Add support for multiple DragAndDrop filesAs the PR depends on another PR #106871 it is tagged as WIP
Here a video showing what the patch allows:
just add the labels as the previous PR and set the dependencies setting to it
I don't have permissions for that
An addon with drag and drop has been developed, but it does a DLL swap out - maybe studying how that works to implement here will help:
https://github.com/mika-f/blender-drag-and-drop
2650becbb7
to7d3b127e8c
WIP: IO: Add support for multiple DragAndDrop filesto WIP: IO: Add support for multiple drag-n-drop files7d3b127e8c
toa60b37d0c5
a60b37d0c5
tobba76c6b74
WIP: IO: Add support for multiple drag-n-drop filesto IO: Add support for multiple drag-n-drop filesRemoved dependency on patch #106871, both are ready to land.
cd9772c1bf
to722b8a3475
This looks fine to me. But will also involve @mont29 as the I/O module owner.
Change generally looks to be going in the right direction.
Will also add @JulianEisel here since he is the dev maintaining the drag&drop code.
@ -767,0 +765,4 @@
path_data->file_type = ED_path_extension_type(paths[0]);
const char *extension = BLI_path_extension(paths[0]);
Is there a reason for using string extension instead of the (already computed)
file_type
here (and then comparing to the result ofED_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.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 thisfile_type
will just you describe the first path.Ah yes, makes sense, better not do any filtering here then indeed.
@ -767,0 +774,4 @@
}
}
const char *tooltip = path_data->paths[0].c_str();
There is no reason to use C-strings here.
std::string
andfmt::format
(extern library we are using since we are not yet on C++20) should be even easier to use.@ -778,3 +801,3 @@
{
if (drag->type != WM_DRAG_PATH) {
return nullptr;
nullptr;
return nullptr;
@ -1040,3 +1073,2 @@
/* 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
This change has nothing to do here? Please avoid unrelated changes in a PR.
@ -1571,2 +1571,2 @@
for (int a = 0; a < stra->count; a++) {
printf("drop file %s\n", stra->strings[a]);
if (stra->count) {
printf("drop file %s\n", stra->strings[0]);
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.Maybe it's better to show which files were discarded
I would say both...
Besides details below, LGTM now.
Here too would be a good opportunity to add unittests to the WM API that is being changed by this patch.
@ -767,0 +774,4 @@
if (path_data->paths.size() > 1) {
std::string path_count = std::to_string(path_data->paths.size());
path_data->tooltip = fmt::format(TIP_("Dragging {} files."), path_count);
No final point in our UI strings:
"Dragging {} files"
@blender-bot build
Great stuff! Looking good mostly, just smaller nitpicks, and some design thoughts.
I think there should be a version of
WM_drag_get_single_path()
that takes a file type, or multiple (eFileSel_File_Types
uses bit flags so that's easy) as argument. Then it would return the first path of a requested type if any. Then you'd probably also want aWM_drag_has_path_file_type()
for the poll functions to use. It's nicer/clearer this way from the API usage side: Instead of "Just give me an arbitrary file and I'll see if I can do anything with it" it becomes "I'd like a file of type X". Most dropboxes would use that instead I guess.@ -1430,3 +1431,3 @@
/**
* 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
The "For internal use only" part doesn't make sense to me, this is a public API.
@ -767,0 +767,4 @@
path_data->file_type = ED_path_extension_type(paths[0]);
for (auto path : paths) {
This is just
const char*
isn't it? In that caseauto
just hides type information with no real benefit, prefer not using it in such cases.@ -0,0 +5,4 @@
#include "testing/testing.h"
/* #bContextStore. */
#include "BKE_context.h"
Doesn't seem to be needed.
Needed by
wmDrag.drop_state.ui_context
, is astd::unique_ptr<bContextStore>
While not a big deal, this include shouldn't be necessary. Committed
e3b3399bcb
so this can be removed now.@ -0,0 +15,4 @@
namespace blender::tests {
TEST(wm_dragdrop, create)
Should mention that this is testing for paths specifically. Suggest something like
TEST(wm_drag, wmDragPath)
.@ -63,6 +65,8 @@
#include "ED_scene.hh"
#include "ED_screen.hh"
#include <fmt/format.h>
Doesn't seem to be needed.
@ -1576,3 +1584,3 @@
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;
Redundant
break
since there's a break a few lines below.@ -1429,1 +1428,3 @@
const char *WM_drag_get_path(const 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);
This is returned by value, so
const
for the return type doesn't make sense (some static analyzers will warn I think).@ -785,0 +797,4 @@
return path_data->paths[0].c_str();
}
const blender::Span<std::string> WM_drag_get_paths(const wmDrag *drag)
Same, remove
const
from return type.This part hasn't been addressed yet, are you planning to do this?
Added with some tests
All @JulianEisel's comments were addressed, and they are relatively minor so assuming this is ready to merge now.
@blender-bot build