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

feat: enable dir listing #47

Merged
merged 16 commits into from
Nov 23, 2018
Merged

feat: enable dir listing #47

merged 16 commits into from
Nov 23, 2018

Conversation

fsdiogo
Copy link
Contributor

@fsdiogo fsdiogo commented Nov 20, 2018

⚠️ PR #44 needs to be merged first!

This is a first step towards the integration of Share Files in Web UI.

Share app is now able to list a mix of files and/or directories:

folders

Things of note:

  • As you can see you're not able to download folders individually, only files
  • To download directories you have to click "Download All" to keep the file structure
  • The two points above are due to the fact that we can only create an archive in the public gateways and not through the core or apis
  • The way "Download All" now works is:
    1. If there are only files in the tree, the "download all" just downloads every file individually, as per PR feat: use jsipfs as fallback instead of public gateways #44
    2. If there are dirs in the tree, the "download all" uses the public gateway to archive and download everything

@fsdiogo fsdiogo self-assigned this Nov 20, 2018
@ghost ghost added the status/in-progress In progress label Nov 20, 2018
@fsdiogo
Copy link
Contributor Author

fsdiogo commented Nov 21, 2018

I'm giving this one a second thought, instead of blocking the download of individual folders, why not use the public gateway to download them too?

Is there any downside?

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Nov 21, 2018

In my last commit I've added the ability to download individual folders, as per my previous comment.

ss

As @olizilla pointed out in IRC, the go/js HTTP API supports archiving files so we'll use the local daemon when possible. Only when using a fallback js-ipfs instance that we'll use the public gateways (only when we need archiving, i.e, folders or download all) 🎉

@olizilla
Copy link
Member

Unless we can figure out a way to make the "download all" multi-click trick work in FF we may want to to tigger a download of the root cid from the url hash, so there is just a single item to download, that contains all the things, regardless of the structure.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Nov 22, 2018

To be honest I think that should be the way to go anyway!

The multi-click trick was just because I didn't know the HTTP API supported archiving files (my bad), but as it does it's best to use that don't you think?

I'll make that change in another PR after merging the two pending ones.

EDIT: I'll do this in this PR.

@olizilla
Copy link
Member

@fsdiogo go for it!

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Nov 22, 2018

Current status:

  • The Download all button uses the HTTP API of a local daemon if possible or the public gateways to archive and download everything, keeping the files struct intact (i.e, no more click-all hacks).
  • Same happens when downloading a folder individually.
  • Progress on Download all is back.

@olizilla
Copy link
Member

Minor UX optimisation, If there is only one thing, The download all button should say "download" and should just download the actual thing, rather than a *.tar.gz.

We should think of a human friendly name for the downloaded tar where there is more than one thing... download_QmXF1xadYVFMogoSFgWf7jCZS2qUsbNB5hcbZWE8Tv5pq1.tar.gz is hard on the brain.

Also when expanded the directory is QmXF1xadYVFMogoSFgWf7jCZS2qUsbNB5hcbZWE8Tv5pq1 which, while accurate, isn't helpful once the thing is outside of IPFS. We don't show the root CID on the UI right now, it only appears in the hash frament of the url, so casual users won't spot that QmZF1... dir came from .../#/QmZF1... share url.

Perhaps when we create it should double wrap, so we can have a named directory like share-ipfs-2018-11-22 ? If the user downloads a bunch of thingds on the same day, the OS will add an index or something sensible, and it connects the folder name in your Downloads foler to how it got there.

@lidel
Copy link
Member

lidel commented Nov 22, 2018

👍 for downloading original unwrapped file if we download all from a share that has single item.

🤔 I have mixed feeling about removing CID from .tar.gz filename and messing with archive contents.

[brain dump commences]

  • Download date is local and incidental, CID is forever:
    • I am worried double wrapping optimizes a single use case but will annoy people that want to keep those .tar.gz around. We should avoid surprises: clicking "Download all" should always just return the same, original archive that people can keep forever.
    • OS filesystem already stores creation timestamp and file managers let users to use it for sorting
    • Adding date as a prefix gives us a prettier name at a cost of rare duplication in my Download folder if I download the same thing on another day. It may be a fair trade-off, but does not require removal of CID.
  • Full CID in filename is an opportunity for exposing general public to the concepts of CID and content-addressing:
    • Indeed, "We don't show the root CID on the UI right now", but.. maybe we should? I agree CIDs look alien at first, but when people are exposed more and more we normalize them: you share link with a friend, they notice similar long identifiers in URL and in filename, and get curious, or at least register the connection.
    • CIDs could be just another thing in the digital landscape, like URLs, barcodes and QR codes ("oh, i am familiar with this weird thing.. well I don't know the details but know where to put it to access stuff behind it").
    • When looking through onboarding lens, would archive_<cid>.tar.gz be more meaningful than download_?
  • Even if we don't want to keep entire CID in filename, we could take last n characters:
    • Better than a date in safeguarding us against accidental duplication or ugly (2) (n) suffixes
    • Can be really short (download_Tv5pq1.tar.gz) so we still have space for adding download date if we really want ;)

[/brain dump]

@olizilla
Copy link
Member

@lidel I here you. My main counter is I don't think the downloaded folder name is the right place to educate the users about CIDs, as it's outside out our realm, and we can't offer any support or guidance. It just looks like IPFS has dumped windows registry keys or UUIDs into my downloads folder.

I think we can iterate on this though and find a decent

When looking through onboarding lens, would archive_.tar.gz be more meaningful than download_?

Even if we don't want to keep entire CID in filename, we could take last n characters:

For a new user, I think the most useful thing is to associate it with the service that caused it to be there... shared-via-ipfs-<7 chars from the end of the cid>.tar.gz would be ok.

I'll have a think about the CID folder name stuff. We can leave that as is for now and revisit once we've got share.ipfs.io battle ready.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Nov 23, 2018

Minor UX optimisation, If there is only one thing, The download all button should say "download" and should just download the actual thing, rather than a *.tar.gz.

👍 for downloading original unwrapped file if we download all from a share that has single item.

I'm not completely sold to that idea. OK with changing Download All > Download if it's a single item, but I like the fact that you always get a tar when clicking that button. If you want the original thing you can just click the individual download button.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Nov 23, 2018

About the CIDs, I think @lidel made some pretty good points there for us not "inceptioning" around the wrapping of content.

Even if we don't want to keep entire CID in filename, we could take last n characters:
Better than a date in safeguarding us against accidental duplication or ugly (2) (n) suffixes
Can be really short (download_Tv5pq1.tar.gz) so we still have space for adding download date if we really want ;)

For a new user, I think the most useful thing is to associate it with the service that caused it to be there... shared-via-ipfs-<7 chars from the end of the cid>.tar.gz would be ok.

I like this. On it.

@fsdiogo
Copy link
Contributor Author

fsdiogo commented Nov 23, 2018

I think this is ready to go.

I'd like to merge this ASAP so share.ipfs.io gets updated with the last changes. (PR #44 didn't get built because of the CI not passing, that has been fixed in this one.)

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like shared-via-ipfs-<7cidchars>.tar.gz and the idea of it associating downloaded archive with the service.

As for concerns about always downloading .tar.gz, I agree it may not be intuitive enough to have icon for direct download. Created #49 with additional way to address that need.

@fsdiogo fsdiogo merged commit 9549008 into master Nov 23, 2018
@ghost ghost removed the status/in-progress In progress label Nov 23, 2018
@fsdiogo fsdiogo deleted the feat/enable-dir-listing branch November 23, 2018 16:01
@fsdiogo fsdiogo mentioned this pull request Nov 26, 2018
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.

3 participants