Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add webkitRelativePath to dragdrop with multiple folders #190

Merged
merged 11 commits into from
Jun 5, 2023

Conversation

cwinland
Copy link
Contributor

@cwinland cwinland commented Mar 8, 2023

Problem: You can select files or a folder. You cannot select multiple. You can drag/drop, but it doesn't do multiple file/folders and subfolders.

Answer:
When using this version, you can drag/drop multiple files and folders with the relative path.
DragnDrop.ts added getFilesAsync (entry point for changes) and readEntryContentAsync to traverse the entries in the folders.

Can drag files and folders at the same time:
Screenshot_ReadyToDragFilesFolders

When dropped, the files maintain the relative path information and add all subfolders:
Screenshot_AfterDrop

When registering the drop event, the function gets the items instead of the files (through getFilesAsync) in order to get more information about the files. Then the results are converted back to FileList / File[] once the data is filled in. This encapsulates all the changes into the new methods of DragnDrop.ts.

DragnDrop.ts, FileReaderComponent.ts, and interface.ts added webkitRelativePath. Still works if webkit is not available.

One of the demos were updated. Did not try to fix anything not related to the PR.

@Tewr
Copy link
Owner

Tewr commented Mar 14, 2023

Hello,
Thank you for your contribution, this looks interesting indeed. I'll try to check this out in the coming days

@cwinland
Copy link
Contributor Author

Hello, Thank you for your contribution, this looks interesting indeed. I'll try to check this out in the coming days

Appreciate you taking the time to review. If you have any questions, please let me know.

@Tewr
Copy link
Owner

Tewr commented Mar 19, 2023

How are you building this? tsc-bundle is failing for me on lib "es2021"

Severity	Code	Description	Project	File	Line	Suppression State
Error	TS6046	Argument for '--lib' option must be: 'es5', 'es6', 'es2015', 'es7', 'es2016', 'es2017', 'es2018', 'es2019', 'es2020', 'esnext', 'dom', 'dom.iterable', 'webworker', 'webworker.importscripts', 'scripthost', 'es2015.core', 'es2015.collection', 'es2015.generator', 'es2015.iterable', 'es2015.promise', 'es2015.proxy', 'es2015.reflect', 'es2015.symbol', 'es2015.symbol.wellknown', 'es2016.array.include', 'es2017.object', 'es2017.sharedmemory', 'es2017.string', 'es2017.intl', 'es2017.typedarrays', 'es2018.asyncgenerator', 'es2018.asynciterable', 'es2018.intl', 'es2018.promise', 'es2018.regexp', 'es2019.array', 'es2019.object', 'es2019.string', 'es2019.symbol', 'es2020.bigint', 'es2020.promise', 'es2020.string', 'es2020.symbol.wellknown', 'esnext.array', 'esnext.symbol', 'esnext.asynciterable', 'esnext.intl', 'esnext.bigint', 'esnext.string', 'esnext.promise'.	Blazor.FileReader	C:\Users\Tor\Source\Repos\BlazorFileReader.PR190\src\Blazor.FileReader\tsconfig.json	20	

My version is NPM typescript-bundle@1.0.18

@cwinland
Copy link
Contributor Author

cwinland commented Mar 20, 2023

How are you building this? tsc-bundle is failing for me on lib "es2021"

Severity	Code	Description	Project	File	Line	Suppression State
Error	TS6046	Argument for '--lib' option must be: 'es5', 'es6', 'es2015', 'es7', 'es2016', 'es2017', 'es2018', 'es2019', 'es2020', 'esnext', 'dom', 'dom.iterable', 'webworker', 'webworker.importscripts', 'scripthost', 'es2015.core', 'es2015.collection', 'es2015.generator', 'es2015.iterable', 'es2015.promise', 'es2015.proxy', 'es2015.reflect', 'es2015.symbol', 'es2015.symbol.wellknown', 'es2016.array.include', 'es2017.object', 'es2017.sharedmemory', 'es2017.string', 'es2017.intl', 'es2017.typedarrays', 'es2018.asyncgenerator', 'es2018.asynciterable', 'es2018.intl', 'es2018.promise', 'es2018.regexp', 'es2019.array', 'es2019.object', 'es2019.string', 'es2019.symbol', 'es2020.bigint', 'es2020.promise', 'es2020.string', 'es2020.symbol.wellknown', 'esnext.array', 'esnext.symbol', 'esnext.asynciterable', 'esnext.intl', 'esnext.bigint', 'esnext.string', 'esnext.promise'.	Blazor.FileReader	C:\Users\Tor\Source\Repos\BlazorFileReader.PR190\src\Blazor.FileReader\tsconfig.json	20	

My version is NPM typescript-bundle@1.0.18

I believe it is actually node that needs to be updated. However, I'm fairly certain that 2021 isn't required if it needs to be removed. https://node.green/#ES2021

When I do node -v, I get v16.13.0.

The minimum is probably ES6

@Tewr
Copy link
Owner

Tewr commented Mar 20, 2023

I finally managed to build this by adding Microsoft.TypeScript.MSBuild as a nuget dependency, not sure why it's needed. But without it I'm running Visual Studio Version 17.6.0 Preview 2.0, and node -v return v18.15.0. Outputs exactly the same .js in any case.

So the first drag n drop demo (DragnDropDivCommon) seems to work well, however the second (DragnDropInputCommon) (that worked well before your PR, I tried the old version from main using the same build setup) does not. IpFileList seems to be empty every time. Any idea of what is broken? Also, a bit less concerning, I'm having a hard time to understand why we need to to a Thread.Sleep, I can see that the first demo is a bit glitchy without it ( I changed it to await Task.Delay(500) though, but I'm not sure what we are waiting for and why?

My feedback is based on the Wasm runtime (Net6/Blazor.FileReader.Wasm.Demo)

@Tewr
Copy link
Owner

Tewr commented Mar 20, 2023

Here is screencaps using the code in the PR, the second example does not print anything
Dragndrop-pr190-test-1

@cwinland
Copy link
Contributor Author

Writing to let you know I'll review and fix.

@cwinland
Copy link
Contributor Author

I finally managed to build this by adding Microsoft.TypeScript.MSBuild as a nuget dependency, not sure why it's needed. But without it I'm running Visual Studio Version 17.6.0 Preview 2.0, and node -v return v18.15.0. Outputs exactly the same .js in any case.

Also, a bit less concerning, I'm having a hard time to understand why we need to to a Thread.Sleep, I can see that the first demo is a bit glitchy without it ( I changed it to await Task.Delay(500) though, but I'm not sure what we are waiting for and why?

The Delay is to release the thread. I noticed that the files return null as they don't have time and might load delayed. Do you know why this might happen?

@cwinland
Copy link
Contributor Author

cwinland commented Mar 23, 2023

Tewr,
I added the MSBuild package to match what you did.

GetFiles was ignoring the fact that the new function was putting the dragdrop files into elementDataTransfers. Changed GetFiles to check for missing files on the element. If so, then it looks at the elementDataTransfers object. This could be done a little differently, so let me know if you want to change that.

I changed the Thread.Sleep to Task.Delay. However, when I tried to remove it, it was inconsistent. I'm looking for a possible await issue, but I have not been able to find it. It seems that sometimes it doesn't get the files into the object fast enough or c# is continuing before JS is finished.

I checked that all demos are working. For the demo, I moved WriteFileInfoOutput to a common place to be called. Only because it was exactly the same code and I found myself updating the same thing in both places. I didn't want to move too much, but it seemed like a good set of code to consolidate.

@Tewr
Copy link
Owner

Tewr commented Mar 28, 2023

I've been debugging this tonight, putting console.logs all over the place, and now I understand.

. What has changed is that your handler is now async.
https://github.com/cwinland/BlazorFileReader/blob/main/src/Blazor.FileReader/script/DragnDrop.ts#L154
Before the event handler would execute in a particular order: first we read the files using dropHandler, then the element's native onDrop event is called.

Now it's different, as dropHandler will now yield to the event queue, we will just execute the code up to to the first await statement and then start executing onDrop (the c# event handler), until it ends or yields, and then we execute the rest. still there are several awaits in the drophandler now, so just yielding once or something like that is not a guarantee for dropHandler to finish its work.

basically, the ondrop event was a place where we could read new files after a drop. It is no longer reliable for this. And I've only ever offered native events: Input.OnChange, and the native Element.OnDrop, to react to a change of the file list. Your proposed change breaks this API, and thats unfortunate. You could argue that the API was broken to begin with, it's probably true, it relies on the exeution ordering of registered events, very fragile.

I'd prefer staying backwards compatible.

In that case we need to opt-in for your behaviour, perhaps with a new parameter somewhere like DropEventsOptions.UseWebkitRelativePath or something . And to reliable read an updated file list in that case, onDrop is useless, we need a new explicit event on IFileReaderRef, something like a FileListChanged event that should fire each time we modify the file list for an element in the code.

What's your thoughts on this.

@cwinland
Copy link
Contributor Author

cwinland commented Mar 29, 2023

I have been playing with it and reverted the workarounds. It looks like the timing issue is because onDrop in c# is being called along with the ondrop in the element. However, I made a few changes to the javascript and was able to get everything to work much better (without the delay) as long as you don't need an immediate reaction such as the file list (before the read file action). In order to make sure we have all the files, I was able to move the delay into the enumerate method instead of the client side of the demo. This means that a dev will not have to worry about it.

Ideally, if you were to have an ondrop that worked without requiring the enumerate method, then an after drop would always pick up those files. Then the delay wouldn't be needed at all.

As for the input specifically, the native input element is handling the files and folders so I was able to put a flag on it to allow webkitdirectory. However, that still uses the native drop and not the code I wrote (unless you register the event). With that, you can only drop files OR folders with the native functionality. However, this can be worked around with adding the ondrop.

Please review and if we still need to opt in, I can add that. I'm not sure that it is needed at this point, but that is up to you. It wouldn't be a bad idea to add an after drop event.

…between using webkitdirectory. Use div / ondrop to drop multiple folders and files.
@Tewr
Copy link
Owner

Tewr commented Mar 30, 2023

I found this old PR #131 that addresses this property already for native inputs. We still need your additional code for non-inputs, but the reasoning in that old PR stands, it would be best of in the dictionary where it resides already

@cwinland
Copy link
Contributor Author

That works. I did it that way because adding the webkitdirectory to the input actually retrieves it there, I thought. I'll make that change as well.

@cwinland
Copy link
Contributor Author

sorry for the delay. I put back the interfaces so that I am using the NonStandardProperties.

@cwinland
Copy link
Contributor Author

I am debugging one issue with the timing with getFilesAsync not finishing before Enumerating the files.

@cwinland
Copy link
Contributor Author

@Tewr This should be good to go. Please let me know if you have any questions.

I simplified the handlers that get the files from the directory structure. This seems to work well. The only thing I was fighting was the "simultaneously fired" onDrop in C#. Instead of the blanket thread yielding, I added a safeguard. The onAfter will not fire until the files are good. If the enumerate is called in the html event, the safeguard is needed. Otherwise, it should be called in the after drop event.

If you were to debug through the call in the drop handler, you will see that nothing fires until getFilesAsync is called due to the way Javascript works. I removed the async call in the handler itself and tried a few things, but it didn't matter as eventually it needed to have an async call eventually.

src/Blazor.FileReader/script/DragnDrop.ts Show resolved Hide resolved
src/Blazor.FileReader/script/DragnDrop.ts Outdated Show resolved Hide resolved
@cwinland
Copy link
Contributor Author

cwinland commented Jun 5, 2023

@Tewr I removed the two requested try catches. There was another set not in the review which I threw so it wouldn't prevent the caller from catching. I believe this is important because it interrupts the loop and you won't know the details of the issue.

@cwinland cwinland requested a review from Tewr June 5, 2023 12:35
@Tewr Tewr merged commit 0374882 into Tewr:main Jun 5, 2023
@Tewr
Copy link
Owner

Tewr commented Jun 5, 2023

Thanks again for your contribution. I'll try to release a new version in the coming days

@cwinland
Copy link
Contributor Author

cwinland commented Jul 3, 2023

@Tewr Do you know when you will be doing a release with these changes? I am waiting to integrate. Thanks!

@Tewr
Copy link
Owner

Tewr commented Jul 4, 2023

Hello. Soon I guess. The typescript build process is currently broken, might take a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants