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

File Upload Widget v2 #2258

Merged
merged 5 commits into from Jun 21, 2019

Conversation

@oscar6echo
Copy link
Contributor

commented Nov 6, 2018

This is an import of the custom widget ipyupload

@xmnlab

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@oscar6echo it seems you have a conflict file in your PR.
maybe would be good if you could rebase your code.

@xmnlab

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@oscar6echo is there a way to get files' URL?

@oscar6echo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Yes this PR is quite old.
I'll try to rebase. but never done that. If you have any tip, that would be useful.

Not sure I understand "a way to get files' URL" ? Do you mean to get a file from a URL ? If so the Python requests packages seems able to to that, right ?

@xmnlab

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

hey @oscar6echo thanks for the quick answer.

I'll try to rebase. but never done that. If you have any tip, that would be useful.

generally, you need to 1) configure a remote upstream and 2) run the rebase with upstream/master.

# check the remote repos you have
git remote -v
# add the upstream
git remote add upstream https://github.com/jupyter-widgets/ipywidgets.git
# get all commits for all remote repos
git fetch --all
# supposing you are in the branch you want to rebase
git rebase -i upstream/master

if there is no conflict, it will show 1) the commits you have, you can just save and exit this file .. and 2) it will show a file with the commits related to this rebase. 3) now you can push with --force

if there are any conflict you should resolve each conflict. Remember you are changing the timeline of you branching ... adding the updates from upstream/master before the changes you did, so when you are resolving conflicts, the HEAD starts with commits from upstream/master ... I hope I could explain me ... let me know if I can help in any way.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Sorry for the delay in merging. @SylvainCorlay is merging things in preparation for a 7.5 release. It would be awesome to get this in.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

It looked like the merge conflict was easy to resolve, so I went ahead and resolved it. I haven't tested or reviewed the PR, though.

@xmnlab

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

that is awesome! thanks @jasongrout ! if I can help in anyway let me know.

@oscar6echo

Not sure I understand "a way to get files' URL" ? Do you mean to get a file from a URL ? If so the Python requests packages seems able to to that, right ?

I mean after the upload is there a way to get the URL of the file? so the user can get this URL and if he want to download the file they can just click on a link.

@thomasaarholt

This comment has been minimized.

Copy link

commented May 24, 2019

@oscar6echo I believe @xmnlab was wondering if it was possible to use FileUpload to get ahold of the filepath of the file(s) being uploaded.

I'm interested in a similar case as to that, where we basically want to let a user navigate their directories, select a file, and then feed the filepath (aka "~/Documents/myfile.csv" to another python function.

So far, I believe this is not possbile by your method, since FileUpload relies on the FileReader Web API, and the API doesn't allow accessing the OS directories directly for security reasons.

I'm wondering if you can confirm this to be correct, and perhaps suggest an alternative? A simple ipywidget Button implemention is the closest I've gotten, but it's not great.

@xmnlab

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

hey @thomasaarholt @oscar6echo ! some example about what I mean is the approach used here: deathbeds/wxyz#5 ... if you try the binder notebook, after you upload a file .. it returns a link if you want to download the uploaded file.

@SylvainCorlay SylvainCorlay added this to the 7.5 milestone Jun 4, 2019

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch from 880dba2 to fc64890 Jun 4, 2019

@mlucool

This comment has been minimized.

Copy link

commented Jun 11, 2019

Can we update the default style? It does not match the rest of ipywidgets.

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch 3 times, most recently from 8f5255d to eaeee82 Jun 18, 2019

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch from eaeee82 to b888280 Jun 18, 2019

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch from b888280 to 8ec07cd Jun 18, 2019

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Rebased the PR and fixed the tests. Will do a more thorough review / cleanup before merging tomorrow.

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch 2 times, most recently from e6aa936 to 34f290c Jun 20, 2019

@rskabelund

This comment has been minimized.

Copy link

commented Jun 20, 2019

Been looking forward to this addition, awesome stuff!

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch 3 times, most recently from 0d63f77 to c3ea8e8 Jun 20, 2019

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

I have iterated a bit on the PR

  • removed the ability to pass arbitrary CSS to comply with the rest of ipywidgets where the DOM structure is not part of the API.
  • re-use the ButtonStyle attribute to allow for some styling with the same API as for the Button widget.
  • added the pre-defined button_style things.
  • iterated a bit on the javascript to not use var that = this thanks to arrow functions, and fixed the logic to count uploaded files.
  • apply the CSS classes with the usual pattern of ipywidgets.
  • make value a read-only trait.
@rskabelund

This comment has been minimized.

Copy link

commented Jun 20, 2019

Just wondering, is there an event handler for when the files are done uploading? Also, can a user change the label of the widget button?

Edit: @SylvainCorlay after looking at the latest commit, it looks like the button would behave like any other button in terms of styling? Is that correct? So in that case, a user could set description and icon as they wish?

@SylvainCorlay SylvainCorlay referenced this pull request Jun 20, 2019
@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Just wondering, is there an event handler for when the files are done uploading? Also, can a user change the label of the widget button?

Yes, the change in the value trait.

Edit: @SylvainCorlay after looking at the latest commit, it looks like the button would behave like any other button in terms of styling? Is that correct? So in that case, a user could set description and icon as they wish?

Absolutely, except that there is a default icon and default description that are not empty.

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

I think we need to iterate a bit more on this.

  • we should make use of binary serialization for the content of the file so that we don't convert binary files to list of hex characters.
  • then (maybe in another PR) we could provide some information about the encoding.
@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

OK, the binary serialization and mime type detection work got me to find a bug in remove_buffers .

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Could it be #1748 ?

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

No, it is really silly.

We need a custom seralizer being identity to prevent the JSON stringification of ArrayBuffer which returns nothing.

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Ah yes, I believe it is a feature, @jasongrout knows the reasoning behind it, and there are good reasons for it I believe, but it has bitten many people (the Image widget has this as well I believe).

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch from c3ea8e8 to 44a2af9 Jun 21, 2019

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

I think that the current state is fine. I am still hesitant about two things

  1. should we remove completely the compression feature from that core widget?

DONE.

  1. should we support the case of text files?

    • At the moment, this uses the browser FileReader.readAsArrayBuffer always, but I presume people will want to upload e.g. CSV files and get the content as a string.
    • The FileReader also has a readAsText method which we could use when the detected MIME type for the File starts with text/...
  • readAsText also takes an optional encoding parameter, defaulting to UTF-8. Can we detect if it is not UTF-8?

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch from 44a2af9 to 2e90a4f Jun 21, 2019

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

* but I presume people will want to upload e.g. CSV files and get the content as a string.

Perhaps a convenience method on the python side that basically is the one line to do the decoding with a default of utf8?

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch 2 times, most recently from 7b23d63 to 2945ab4 Jun 21, 2019

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

This is ready to go. Will merge in a little.

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Perhaps a convenience method on the python side that basically is the one line to do the decoding with a default of utf8?

As per discussion on gitter. We may add an example with decode('utf8').

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

@jasongrout

li_content    --->    data
li_metadata   --->    metadata

?

@rskabelund

This comment has been minimized.

Copy link

commented Jun 21, 2019

should we support the case of text files?

At the moment, this uses the browser FileReader.readAsArrayBuffer always, but I presume people will want to upload e.g. CSV files and get the content as a string.
The FileReader also has a readAsText method which we could use when the detected MIME type for the File starts with text/...
readAsText also takes an optional encoding parameter, defaulting to UTF-8. Can we detect if it is not UTF-8?

@SylvainCorlay When I attempted this widget before I used a read_mode attribute with a default as 'text'.

if (read_mode == "binary") {
    file_readers[i].readAsBinaryString(file);
}            
else if (read_mode == "buffer") {
    file_readers[i].readAsArrayBuffer(file);
}
else {
    file_readers[i].readAsText(file);
}

In my use case, yeah reading csv files is exactly what I would do, i.e. StringIO->pandas.read_csv()...

@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Thanks @rskabelund! The issue with reading as text is how to deal with encoding.

When only uploading binary is that the choice is left to the user.

@SylvainCorlay SylvainCorlay force-pushed the oscar6echo:fileupload branch from 723df20 to 9e975f6 Jun 21, 2019

@SylvainCorlay SylvainCorlay merged commit df56513 into jupyter-widgets:master Jun 21, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stojan-jovic

This comment has been minimized.

Copy link

commented Jul 27, 2019

Thanks guys for this widget (especially to author), appreciate your work. I assume that this will be available from the next release? I tried standalone library, but in my case huge downside was inability to customize button name. Glad to hear that this will be fixed in the official version...

@jasongrout

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

The upload widget is in the 7.5 release.

@stojan-jovic

This comment has been minimized.

Copy link

commented Jul 28, 2019

I missed it somehow in documentation, it's really there.

Oh god, I spent whole one day to implement the same thing through the combination of HTML and Javascript, only to have custom name in button. :( But ok...

Thanks Jason one more time!

@hainm

This comment has been minimized.

Copy link

commented Jul 28, 2019

It’s good to learn new thing anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.