Indicate that a file is uploading because user has chosen a folder #2605

chiefGui · 2020-10-29T03:01:43Z

Hello, there!

The idea behind this PR is simple: when a file is being added through a folder selection from a provider using ProviderView, I need an indication of that. I am reconstructing folder structure from files uploaded via Dropbox/Google Drive, but unfortunately, if I don't know whether the file was picked because the user has selected a folder, I'll be always reconstructing the folder path exposed by the mentioned provider.

This line of code is all that's needed to achieve that.

Please, let me know if you need further clarification.

This reverts commit 5f41acb.

chiefGui · 2020-10-29T13:04:48Z

packages/@uppy/provider-views/src/CloseWrapper.js

@@ -1,4 +1,4 @@
-const { h, Component } = require('preact')
+const { Component } = require('preact')


Just FYI, I had to remove this unused import because Travis/npm run lint were complaining.

chiefGui · 2020-10-29T13:17:56Z

FWIW, the force-push was because I saved the documentation file (see at cab9f6f) with custom formatting, and pushed with it. Also, the revert for the "Release" commit (74d1c6b) and the "Release" commit itself (
5f41acb) were stuff I pushed after playing a bit with Uppy last night (mistakenly, lol).

ifedapoolarewaju · 2020-10-30T05:50:50Z

Hi @chiefGui, I'd like to get some more clarifications here, what exactly is happening when you say "reconstructing folder structure", and how are you able to achieve it when a file is selected directly vs when a file is selected through a parent folder?

chiefGui · 2020-10-30T14:25:56Z

Hello @ifedapoolarewaju, thank you for your answer.

Pretty much, Dropbox files are giving me remote.url, which displays the relativePath of the file. I'm grabbing that data, decoding it, and using it to recreate the folder structure of such file.

Turns out I can't differentiate a "standalone" file, and a file from a folder, meaning direct files are always recreating their parent folder structure and that's not desired—I only want to recreate the structure if the files uploaded were selected through folder picking.

What do you think? Am I making sense?

lakesare · 2020-10-30T15:05:53Z

@chiefGui, thanks, that makes sense!
I think we should expose the file.relativePath for all remote providers in the future (you can't see .relativePath for google drive e.g., but we do have access to it internally).
.isFromFolder is a neat idea!

One issue with this PR is in docs we mention that .isFromFolder is accessible via file.isFromFolder, but it's in fact in file.data.isFromFolder.

@arturi, @ifedapoolarewaju,

How about we name it .isSelectedFromFolder instead to indicate that it's not about whether the file was nested, - it's about the way we selected it.
And we should place .isFromFolder somewhere else 🤔.
It should be somewhere close to the .relativePath (when we add it).
Atm we have file.meta.relativePath for dropped files. Was it ever a semanticaly sane place to put relativePath into, mb we should be assigning .relativePath & .isFromFolder directly to the file object, what do you think?

chiefGui · 2020-10-30T16:16:48Z

Hey @lakesare, thank you for the quick reply.

Yes, hands down on exposing relativePath 💯. The other day I was reading Drive/Dropbox APIs and I saw they exposed a file's folder structure, but that wasn't bridged by Uppy so I was like, hmmm maybe they have it buried somewhere so I found it through .url lol.

Now, am not sure if those points you raised were internal-only but I'll participate anyways for what it's worth.

isSelectedFromFolder sounds better to me, for sure. +1 on that, if you ask me.
After thoroughly reading your APIs, docs, and working with Uppy for 2 years now, I'd say .meta(.relativePath|.isSelectedFromFolder) would be the cleverest. To me, both relativePath and isSelectedFromFolder are about the file, and not children [data] of the file. What do you think?

Asking these for me to update the PR accordingly.

chiefGui · 2020-10-30T17:01:40Z

Oh, before I forget. Do you want me to expose relativePath through this PR or would you want to separate the two functionalities? Thanks!

cc @lakesare

lakesare · 2020-11-02T15:35:33Z

@chiefGui, sorry for the delay, talked over it in Slack.

isSelectedFromFolder sounds better to me, for sure. +1 on that, if you ask me.

Deal then 👍

After thoroughly reading your APIs, docs, and working with Uppy for 2 years now, I'd say .meta(.relativePath|.isSelectedFromFolder) would be the cleverest. To me, both relativePath and isSelectedFromFolder are about the file, and not children [data] of the file. What do you think?

We discussed this, and agreed that file.meta is the best place for .relativePath and .isSelectedFromFolder, because file.meta is for data that will be sent to backend, and file itself is for temporary information (like upload status).
We don't strictly follow this logic at the moment, but we plan to move in that direction.
If you have some time on your hands we'd appreciate you adding the .relativePath to meta for remote providers, - but .isSelectedFromFolder is appreciated on its own too (once you change the name and remove it from the file object in docs).

This reverts commit cab9f6f.

lakesare · 2020-11-17T20:13:43Z

@chiefGui, is the PR ready for review?

chiefGui · 2020-11-18T16:36:53Z

Hey @lakesare, sorry for the delay. Yes, I guess it's now ready for review, although there are a few considerations I want to bring to the table.

This PR is working and tested in Dropbox, Google Drive and OneDrive and so far I only had successful results.
After working a little bit more on this PR, I think we can safely get rid of isSelectedFromFolder. Pretty much, what I'm proposing here is that if a file has a relativePath, then it also means this file was uploaded via a folder. I left the variable there just in case you want to keep it for clarity or else, but we're good to remove it.
Another thing to take into account is that this is a 100% front-end solution, and I feel it might break with pagination, although as mentioned, I haven't seen any issues so far. To make this a 100% reliable implementation, I'd say we'd need to retrieve the currentFolderName with the response of /list from the server. That because the relativePath Uppy is giving me is the file ID, and only Dropbox is using the folder's name as ID. Another option would be handling relative path entirely on the server, especially because it knows more than the front-end does.

It's been a while since I last touched this code. I wrote the code you see here and published our own Uppy's version shortly after just to respect our deadlines, and just now I've got a chance to give you feedback—so pardon me for any confusing explanation or rationale.

In a nutshell, that's all about it. Let me know what you think and count on me to update whatever is needed to match your expectations to have this update out. 🚀

chiefGui · 2020-11-18T16:41:33Z

packages/@uppy/provider-views/src/ProviderView/ProviderView.js

    files = files || []
    return new Promise((resolve, reject) => {
      this.provider.list(path).then((res) => {
+        let itemsRelativePath
+
+        if (meta) {


My idea on having this condition here is that if there's a meta, then there's a previousRelativePath and a currentFolder. That's why I skipped checking those individual properties.

chiefGui · 2020-11-18T16:49:58Z

packages/@uppy/provider-views/src/ProviderView/ProviderView.js

@@ -430,11 +447,23 @@ module.exports = class ProviderView {
    }
  }

-  listAllFiles (path, files = null) {
+  listAllFiles (path, files = null, meta = undefined) {
    files = files || []
    return new Promise((resolve, reject) => {
      this.provider.list(path).then((res) => {


This res is where we'd need currentFolderName. Currently, if moreFiles === true, this implementation won't reproduce the correct parent's folder names for Google Drive and OneDrive since their IDs are unique, hashed strings instead of folder names (IMHO, Dropbox took the cleverest approach here).

FWIW, the relativePath in the data object is giving /{fileId}; and this is the reason that the above statement is relevant.

Read more at #2605 (comment).

chiefGui · 2022-05-02T20:52:10Z

I am already considering this as a dead pull request that won't go anywhere but the graveyard. However, am curious - did you guys fix the issue and just forgot to flag here?

Murderlon · 2022-05-03T10:13:18Z

Hi, sorry your contribution went stale. Looking at this PR I'm not convinced about the current state to merge it. It seems like quite a lot of changes for something that in my mind only needs something similar to what you said:

2. I think we can safely get rid of isSelectedFromFolder. Pretty much, what I'm proposing here is that if a file has a relativePath, then it also means this file was uploaded via a folder. I left the variable there just in case you want to keep it for clarity or else, but we're good to remove it.

I'm not a fan of opening the box on adding things like isSelectedFromFolder and instead do something more agnostic like relativePath and do with that what you like. The pattern of previousFilePath also feels off to me, do we need that?

If we only add relativePath for remote files from folders, would that be enough? I can focus on making it land if we agree.

chiefGui · 2022-05-04T16:18:08Z

@Murderlon My solution is completely front-end oriented. Given the context I was in, that's the best I could achieve—and am telling you this to quite justify the decisions I took in this Pull Request.

The above said I agree with you: the less noise we have on the (front-end) API (by omitting things like previousFilePath and isSelectedFromFolder), the better.

Meaning...

If we only add relativePath for remote files from folders, would that be enough? I can focus on making it land if we agree.

may work if, with it, we can reconstruct the folder directory of a certain uploading/uploaded file.

Murderlon · 2022-05-05T10:18:21Z

may work if, with it, we can reconstruct the folder directory of a certain uploading/uploaded file.

I can take a look at this but I have some other things to do first. One could also say this is a server-side concern, for which we could add it to the response for every provider, such as here for Google Drive:

uppy/packages/@uppy/companion/src/server/provider/drive/index.js

Lines 31 to 52 in d42badf

    
           function adaptData (listFilesResp, sharedDrivesResp, directory, query, showSharedWithMe) { 
        
             const adaptItem = (item) => ({ 
        
               isFolder: adapter.isFolder(item), 
        
               icon: adapter.getItemIcon(item), 
        
               name: adapter.getItemName(item), 
        
               mimeType: adapter.getMimeType(item), 
        
               id: adapter.getItemId(item), 
        
               thumbnail: adapter.getItemThumbnailUrl(item), 
        
               requestPath: adapter.getItemRequestPath(item), 
        
               modifiedDate: adapter.getItemModifiedDate(item), 
        
               size: adapter.getItemSize(item), 
        
               custom: { 
        
                 isSharedDrive: adapter.isSharedDrive(item), 
        
                 imageHeight: adapter.getImageHeight(item), 
        
                 imageWidth: adapter.getImageWidth(item), 
        
                 imageRotation: adapter.getImageRotation(item), 
        
                 imageDateTime: adapter.getImageDate(item), 
        
                 videoHeight: adapter.getVideoHeight(item), 
        
                 videoWidth: adapter.getVideoWidth(item), 
        
                 videoDurationMillis: adapter.getVideoDurationMillis(item), 
        
               }, 
        
             })

chiefGui · 2022-05-05T16:49:03Z

My vote would be let's go 100% server-side if you ask me.

Another thing to take into account is that this is a 100% front-end solution, and I feel it might break with pagination, although as mentioned, I haven't seen any issues so far. To make this a 100% reliable implementation, I'd say we'd need to retrieve the currentFolderName with the response of /list from the server. That because the relativePath Uppy is giving me is the file ID, and only Dropbox is using the folder's name as ID. Another option would be handling relative path entirely on the server, especially because it knows more than the front-end does.

I mean, it's yet unknown to me what the consequences are of relying on a front-end solution for this specific case (although my solution worked just fine for the last 2 years).

Murderlon · 2022-08-23T09:36:47Z

I created a new issue for this, closing this PR.

closes #4486 #4034 related #2605

chiefGui added 6 commits October 28, 2020 23:35

provider-views: indicate a uploaded file is from a folder

58a5eef

Release

5f41acb

Remove unused import

cd60ac6

Expose isFromFolder as a type

e07bbfd

Revert "Release"

74d1c6b

This reverts commit 5f41acb.

Add documentation

cab9f6f

chiefGui force-pushed the master branch from d223c1e to cab9f6f Compare October 29, 2020 13:01

chiefGui changed the title ~~Indicate a file is uploaded because user has chosen a folder~~ Indicate that a file is uploading because user has chosen a folder Oct 29, 2020

chiefGui commented Oct 29, 2020

View reviewed changes

lakesare self-requested a review October 29, 2020 22:08

chiefGui added 4 commits November 4, 2020 18:49

provider-views: add relativePath and isSelectedFromFolder

ac8b6d9

add gates to relativePath construction

ade9663

Revert "Add documentation"

30051dd

This reverts commit cab9f6f.

Merge branch 'master' of https://github.com/transloadit/uppy

10c4db1

lakesare marked this pull request as draft November 6, 2020 01:27

Remove redundant gate

d84a6b2

chiefGui commented Nov 18, 2020

View reviewed changes

arturi assigned lakesare Dec 8, 2020

arturi requested review from Murderlon and removed request for lakesare October 4, 2021 13:15

arturi assigned Murderlon and unassigned lakesare Oct 4, 2021

aduh95 force-pushed the main branch from 39f95a0 to 5567be7 Compare August 16, 2022 15:52

Murderlon mentioned this pull request Aug 23, 2022

Add folder path to response of remote files #4034

Closed

2 tasks

Murderlon closed this Aug 23, 2022

mifi added a commit that referenced this pull request Jun 29, 2023

implement relativePath for remote files

c05a8c4

closes #4486 #4034 related #2605

mifi mentioned this pull request Jun 29, 2023

Remote file paths #4537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indicate that a file is uploading because user has chosen a folder #2605

Indicate that a file is uploading because user has chosen a folder #2605

chiefGui commented Oct 29, 2020 •

edited

Loading

chiefGui Oct 29, 2020

chiefGui commented Oct 29, 2020 •

edited

Loading

ifedapoolarewaju commented Oct 30, 2020

chiefGui commented Oct 30, 2020 •

edited

Loading

lakesare commented Oct 30, 2020 •

edited

Loading

chiefGui commented Oct 30, 2020

chiefGui commented Oct 30, 2020 •

edited

Loading

lakesare commented Nov 2, 2020

lakesare commented Nov 17, 2020

chiefGui commented Nov 18, 2020 •

edited

Loading

chiefGui Nov 18, 2020

chiefGui Nov 18, 2020

chiefGui commented May 2, 2022 •

edited

Loading

Murderlon commented May 3, 2022 •

edited

Loading

chiefGui commented May 4, 2022

Murderlon commented May 5, 2022

chiefGui commented May 5, 2022 •

edited

Loading

Murderlon commented Aug 23, 2022

		@@ -1,4 +1,4 @@
		const { h, Component } = require('preact')
		const { Component } = require('preact')

Indicate that a file is uploading because user has chosen a folder #2605

Indicate that a file is uploading because user has chosen a folder #2605

Conversation

chiefGui commented Oct 29, 2020 • edited Loading

chiefGui Oct 29, 2020

Choose a reason for hiding this comment

chiefGui commented Oct 29, 2020 • edited Loading

ifedapoolarewaju commented Oct 30, 2020

chiefGui commented Oct 30, 2020 • edited Loading

lakesare commented Oct 30, 2020 • edited Loading

chiefGui commented Oct 30, 2020

chiefGui commented Oct 30, 2020 • edited Loading

lakesare commented Nov 2, 2020

lakesare commented Nov 17, 2020

chiefGui commented Nov 18, 2020 • edited Loading

chiefGui Nov 18, 2020

Choose a reason for hiding this comment

chiefGui Nov 18, 2020

Choose a reason for hiding this comment

chiefGui commented May 2, 2022 • edited Loading

Murderlon commented May 3, 2022 • edited Loading

chiefGui commented May 4, 2022

Murderlon commented May 5, 2022

chiefGui commented May 5, 2022 • edited Loading

Murderlon commented Aug 23, 2022

chiefGui commented Oct 29, 2020 •

edited

Loading

chiefGui commented Oct 29, 2020 •

edited

Loading

chiefGui commented Oct 30, 2020 •

edited

Loading

lakesare commented Oct 30, 2020 •

edited

Loading

chiefGui commented Oct 30, 2020 •

edited

Loading

chiefGui commented Nov 18, 2020 •

edited

Loading

chiefGui commented May 2, 2022 •

edited

Loading

Murderlon commented May 3, 2022 •

edited

Loading

chiefGui commented May 5, 2022 •

edited

Loading