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
Contributor

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:

  1. The files are filtered according to the extension of the first
    selected file.
  2. Not all operators that import files support importing multiple files,
    so they will still import one.
  3. Changes must be made to allow importers to read all file paths.

Here is a video showing what this patch would allow along with this other patch #106871 (a change is needed to get all filepaths)

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: 1. The files are filtered according to the extension of the first selected file. 2. Not all operators that import files support importing multiple files, so they will still import one. 3. Changes must be made to allow importers to read all file paths. Here is a [video](https://projects.blender.org/attachments/02969f77-8900-46a2-88ab-da26428251c9) showing what this patch would allow along with this other patch #106871 (a change is needed to get all filepaths)
Guillermo Venegas added 2 commits 2023-04-22 02:34:37 +02:00
10933d0d27 UI: Import obj files by drag and drop
The patch allows users to quickly import obj files by drag and drop. For
quick setup of the import, a popup dialog is displayed allowing the user
to configure the import or use a operator preset.
2650becbb7 IO: Add support for multiple DragAndDrop files
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 only imports
the first selected file.

The patch adds support for drag and drop multiple files from external
windows.

Notes:
1. The files are filtered according to the extension of the first
selected file.
2. Not all operators that import files support importing multiple files,
so they will still import one.
Guillermo Venegas changed title from IO: Add support for multiple DragAndDrop files to WIP: IO: Add support for multiple DragAndDrop files 2023-04-22 02:34:48 +02:00
Author
Contributor

As the PR depends on another PR #106871 it is tagged as WIP

Here a video showing what the patch allows:

As the PR depends on another PR [#106871](https://projects.blender.org/blender/blender/pulls/106871) it is tagged as WIP Here a video showing what the patch allows:
First-time contributor

just add the labels as the previous PR and set the dependencies setting to it

just add the labels as the previous PR and set the dependencies setting to it
Author
Contributor

I don't have permissions for that

I don't have permissions for that
Contributor

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

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](https://github.com/mika-f/blender-drag-and-drop)
Guillermo Venegas force-pushed dragndrop-files from 2650becbb7 to 7d3b127e8c 2023-05-20 16:59:40 +02:00 Compare
Guillermo Venegas changed title from WIP: IO: Add support for multiple DragAndDrop files to WIP: IO: Add support for multiple drag-n-drop files 2023-06-07 20:18:05 +02:00
Guillermo Venegas force-pushed dragndrop-files from 7d3b127e8c to a60b37d0c5 2023-06-07 22:03:16 +02:00 Compare
Guillermo Venegas force-pushed dragndrop-files from a60b37d0c5 to bba76c6b74 2023-06-08 02:52:42 +02:00 Compare
Guillermo Venegas changed title from WIP: IO: Add support for multiple drag-n-drop files to IO: Add support for multiple drag-n-drop files 2023-06-08 02:58:52 +02:00
Author
Contributor

Removed dependency on patch #106871, both are ready to land.

Removed dependency on patch #106871, both are ready to land.
Iliya Katushenock added this to the User Interface project 2023-06-10 11:23:23 +02:00
Iliya Katushenock added the
Interest
Pipeline, Assets & IO
label 2023-06-10 11:23:56 +02:00
Guillermo Venegas force-pushed dragndrop-files from cd9772c1bf to 722b8a3475 2023-08-08 22:00:33 +02:00 Compare
Guillermo Venegas added 1 commit 2023-08-08 22:53:29 +02:00
Guillermo Venegas added 2 commits 2023-08-18 19:57:24 +02:00
Guillermo Venegas added 1 commit 2023-08-19 04:56:58 +02:00
Guillermo Venegas added 2 commits 2023-10-26 21:43:12 +02:00
Guillermo Venegas added 1 commit 2023-10-26 21:45:12 +02:00
Guillermo Venegas added 1 commit 2023-10-26 22:36:13 +02:00
Brecht Van Lommel requested review from Bastien Montagne 2023-11-03 18:44:11 +01:00
Brecht Van Lommel approved these changes 2023-11-03 18:44:50 +01:00
Brecht Van Lommel left a comment
Owner

This looks fine to me. But will also involve @mont29 as the I/O module owner.

This looks fine to me. But will also involve @mont29 as the I/O module owner.
Bastien Montagne requested changes 2023-11-07 16:38:54 +01:00
Bastien Montagne left a comment
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.

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 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.
Author
Contributor

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.
brecht marked this conversation as resolved
@ -767,0 +774,4 @@
}
}
const char *tooltip = path_data->paths[0].c_str();

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.
guishe marked this conversation as resolved
@ -778,3 +801,3 @@
{
if (drag->type != WM_DRAG_PATH) {
return nullptr;
nullptr;

return nullptr;

`return nullptr;`
guishe marked this conversation as resolved
@ -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.

This change has nothing to do here? Please avoid unrelated changes in a PR.
guishe marked this conversation as resolved
@ -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.

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.
Author
Contributor

Maybe it's better to show which files were discarded

Maybe it's better to show which files were discarded

I would say both...

I would say both...
guishe marked this conversation as resolved
Bastien Montagne requested review from Julian Eisel 2023-11-07 16:39:12 +01:00
Guillermo Venegas added 1 commit 2023-11-07 19:32:01 +01:00
Guillermo Venegas added 1 commit 2023-11-07 19:35:14 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
b2b60973ae
format
Bastien Montagne approved these changes 2023-11-08 18:27:10 +01:00
Bastien Montagne left a comment
Owner

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.

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"

No final point in our UI strings: `"Dragging {} files"`
guishe marked this conversation as resolved

@blender-bot build

@blender-bot build
Guillermo Venegas added 3 commits 2023-11-10 15:10:30 +01:00
Julian Eisel requested changes 2023-11-22 12:55:03 +01:00
Julian Eisel left a comment
Member

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 a WM_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.

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 a `WM_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
Member

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.
guishe marked this conversation as resolved
@ -767,0 +767,4 @@
path_data->file_type = ED_path_extension_type(paths[0]);
for (auto path : paths) {
Member

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.
guishe marked this conversation as resolved
@ -0,0 +5,4 @@
#include "testing/testing.h"
/* #bContextStore. */
#include "BKE_context.h"
Member

Doesn't seem to be needed.

Doesn't seem to be needed.
Author
Contributor

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

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.
brecht marked this conversation as resolved
@ -0,0 +15,4 @@
namespace blender::tests {
TEST(wm_dragdrop, create)
Member

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)`.
guishe marked this conversation as resolved
@ -63,6 +65,8 @@
#include "ED_scene.hh"
#include "ED_screen.hh"
#include <fmt/format.h>
Member

Doesn't seem to be needed.

Doesn't seem to be needed.
guishe marked this conversation as resolved
@ -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;
Member

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

Redundant `break` since there's a break a few lines below.
guishe marked this conversation as resolved
Julian Eisel reviewed 2023-11-22 13:07:29 +01:00
@ -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);
Member

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).
guishe marked this conversation as resolved
@ -785,0 +797,4 @@
return path_data->paths[0].c_str();
}
const blender::Span<std::string> WM_drag_get_paths(const wmDrag *drag)
Member

Same, remove const from return type.

Same, remove `const` from return type.
guishe marked this conversation as resolved
Guillermo Venegas added 3 commits 2023-11-24 16:31:17 +01:00
Member

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 a WM_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.

This part hasn't been addressed yet, are you planning to do this?

> 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 a `WM_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. This part hasn't been addressed yet, are you planning to do this?
Guillermo Venegas added 2 commits 2023-11-28 18:13:05 +01:00
Author
Contributor

Added with some tests

Added with some tests
Brecht Van Lommel approved these changes 2023-12-12 15:20:04 +01:00
Brecht Van Lommel requested review from Julian Eisel 2023-12-12 15:20:41 +01:00
Brecht Van Lommel added 1 commit 2023-12-12 15:37:06 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
a6e94d13d1
Merge remote-tracking branch 'origin/main' into HEAD

All @JulianEisel's comments were addressed, and they are relatively minor so assuming this is ready to merge now.

@blender-bot build

All @JulianEisel's comments were addressed, and they are relatively minor so assuming this is ready to merge now. @blender-bot build
Brecht Van Lommel merged commit c00c8b1b37 into main 2023-12-12 18:46:22 +01:00
Brecht Van Lommel deleted branch dragndrop-files 2023-12-12 18:46:25 +01:00
Sign in to join this conversation.
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107230
No description provided.