Tewr / BlazorFileReader

Library for creating read-only file streams from file input elements or drop targets in Blazor.
MIT License
425 stars 61 forks source link

Add webkitRelativePath to dragdrop with multiple folders #190

Closed cwinland closed 1 year ago

cwinland commented 1 year ago

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 commented 1 year ago

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

cwinland commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

cwinland commented 1 year ago

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

cwinland commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

Tewr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

cwinland commented 1 year ago

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

cwinland commented 1 year ago

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

cwinland commented 1 year ago

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

Tewr commented 1 year ago

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

cwinland commented 1 year ago

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

Tewr commented 1 year ago

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