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

[Feature request] drag-and-drop files to markdown cells for attachments #5744

Closed
yoichi-kazama opened this issue Dec 9, 2018 · 22 comments · Fixed by #5913
Closed

[Feature request] drag-and-drop files to markdown cells for attachments #5744

yoichi-kazama opened this issue Dec 9, 2018 · 22 comments · Fixed by #5913

Comments

@yoichi-kazama
Copy link

@yoichi-kazama yoichi-kazama commented Dec 9, 2018

Feature request
Cell attachments are an important feature to create self-contained portable notebooks, but now we have to switch to Classic Notebook when creating attachments. It would be nice if we can:

  1. Drop a file outside the wb browser onto a markdown cell for local file attachments (same as Classic Notebook),
  2. Drop a file from the sidebar filebrowser to a markdown cell for remote file attachments.
@jasongrout jasongrout added this to the Future milestone Dec 11, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 11, 2018

Thanks. Both of those sound like excellent suggestions!

@vidartf
Copy link
Member

@vidartf vidartf commented Dec 12, 2018

Xref #4306 for a related discussion.

@agoose77
Copy link
Contributor

@agoose77 agoose77 commented Dec 13, 2018

I've created an extension to restore the original UI for attachments here. I've also added the ability to add from the File Browser Tree context menu. I'm going to look into adding the clipboard paste feature too, but I could use some input on this, as I'm not sure where it is appropriate to add the event handler (as it seems one must handle this through native JS code rather than any JupyterLab API?)

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Jan 12, 2019

Hi, @agoose77 , do you mind if I start looking into this too >

@agoose77
Copy link
Contributor

@agoose77 agoose77 commented Jan 12, 2019

Hi @Madhu94, not at all! I've finished working on this for now, with my extension restoring the original Notebook UI, and adding a right-click handler to image files. I think longer term, it might be better to perhaps add this to the cell-tools panel, but really this needs a proper design task.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jan 12, 2019

You are becoming quite the expert at the drag/drop API @Madhu94 😄

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Jan 27, 2019

@ian-r-rose Ha ha :) I've opened a WIP PR here - #5913

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Jan 30, 2019

Although the nbformat spec mentions attachments for both raw and markdown cells, both the notebook implementation (jupyter/notebook#621) and the work done by @vidartf in #4373, work only with markdown cells ?

@agoose77
Copy link
Contributor

@agoose77 agoose77 commented Jan 30, 2019

Have you tested this? I recall it working when I developed the attachments plugin, but I could be misremembering.

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Jan 31, 2019

@vidartf
Copy link
Member

@vidartf vidartf commented Feb 5, 2019

@Madhu94 The code in #4373 does add support for attachments on the model of raw cells. The core renderer for raw cells does not use theses attachments, but other extensions might want to use them. One example for a raw cell that might want an attachment is latex cells I think?

@vidartf
Copy link
Member

@vidartf vidartf commented Mar 4, 2019

@agoose77 Are you happy with the stability of the extension? I had a look at the code, and it looks good enough that I think the way forward is to just do one review pass on it as a PR into core. I think it could best be added as another plugin to the notebook-extension package. The index file there desperately needs to be split into some sub-modules, but that is probably best done after such a PR. Thoughts?

@vidartf
Copy link
Member

@vidartf vidartf commented Mar 4, 2019

Or maybe the index.ts file could be put as "attachments.ts" in the notebook-extension package, and then simply imported into the index for re-export. That should be less work for you as well.

@vidartf
Copy link
Member

@vidartf vidartf commented Mar 4, 2019

@Madhu94 / @agoose77 I see that the format for the mime data on the paste event are different between the drag-and-drop PR (#5913) and the extension (https://github.com/agoose77/jupyterlab-attachments). Do you guys have any opinions about how to best reconcile these?

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Mar 4, 2019

I believe they handle different cases - (correct me if I'm wrong @agoose77 ) - The PR in #5913 handles cases where you copy the image (as datauri), from a webpage or the imageviewer in your system, and paste it into your notebook. @agoose77's extension handles pasting from filebrowser, using a custom mime type for the clipboard, and the user doesn't have to open the image document to do this. It also seems to handle pasting into multiple selected cells, which the PR doesn't.

@agoose77
Copy link
Contributor

@agoose77 agoose77 commented Mar 4, 2019

@Madhu94 sorry for the delay, I've been afk for a bit.
@vidartf

Or maybe the index.ts file could be put as "attachments.ts" in the notebook-extension package, and then simply imported into the index for re-export. That should be less work for you as well.

I'm not a seasoned JS developer, so I shouldn't advise on the best case RE notebook-extension, but the suggestion to add to a new file seems reasonable over adding to the index.ts there.

Furthermore, I think it might be a good idea to push my extension further - I think we should be adding more functionality to the cell-tools panel, e.g. a list box of the current cell attachments, and the ability to delete / copy / paste them from there.

@vidartf

@Madhu94 / @agoose77 I see that the format for the mime data on the paste event are different between the drag-and-drop PR (#5913) and the extension (https://github.com/agoose77/jupyterlab-attachments). Do you guys have any opinions about how to best reconcile these?

At the moment, I don't see that they're different anywhere? Right, I implemented the ability to copy attachments and paste them between cells using the clipboard, and hence although @Madhu94 and I share the same MIME type, mine stores MIME bundles whereas @Madhu94 stores a closure over a promise for the file contents. I think we should be using different MIME types here - they represent different use cases.

@agoose77
Copy link
Contributor

@agoose77 agoose77 commented Mar 5, 2019

@Madhu94 would you be happy to rename your MIME type to something like "vnd.jupyter.files" or something? Otherwise, think my extension might need some changes to improve functionality, but it shouldn't stop this PR being approved.

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Mar 5, 2019

@agoose77 I'm using a custom mime-type for dragging from filebrowser, so I can rename that. But as far as the paste functionality is concerned, I'm not using a custom mime-type - when you do Ctrl + C on an image or do right click + copy image url for an image, a mime-type of image/png (or whatever type the image is) gets copied on to the clipboard. Let me know if the last sentence was unclear? I believe this is the difference @vidartf means.

@vidartf
Copy link
Member

@vidartf vidartf commented Mar 6, 2019

Ah, sorry for the confusion. I see that the code in #5913 uses the application/vnd.jupyter.attachments mime-type for drag-and-drop, not copy-paste. Still, I have some concerns about the closures having the same mime-type as direct data would. Maybe a parameter string is the best way to modify the mime-type? E.g. application/vnd.jupyter.attachments;closure=true ?

@agoose77
Copy link
Contributor

@agoose77 agoose77 commented Mar 6, 2019

Ah, sorry for the confusion. I see that the code in #5913 uses the application/vnd.jupyter.attachments mime-type for drag-and-drop, not copy-paste. Still, I have some concerns about the closures having the same mime-type as direct data would. Maybe a parameter string is the best way to modify the mime-type? E.g. application/vnd.jupyter.attachments;closure=true ?

Absolutely - this was why I suggested to @Madhu94 to use a different MIME type for the file browser, as I think our two use cases are different (mine denotes a collection of attachment MIME bundles whereas @Madhu94 stores closures-to-promises). I don't think there's any benefit from trying to somehow coerce them into the same format (e.g. changing my extension to store functions).

@Madhu94 based upon @vidartf's last reply, I think we're on the same page? Let me know if you want to clarify things further :) .

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Mar 9, 2019

Maybe a parameter string is the best way to modify the mime-type? E.g. application/vnd.jupyter.attachments;closure=true

@vidartf I was hoping that during review, a better way to store the drag data would be suggested :)
But if we are leaving the closures there, I'm fine with changing the mime type name.

@agoose77 Yes, we are :)

@vidartf
Copy link
Member

@vidartf vidartf commented Apr 4, 2019

@Madhu94 / @agoose77 :
In principle, for lab to be feature compatible with classic notebook, it would need some method for adding (ideally also removing) attachments. I think the PR of @Madhu94 (#5913) should cover most of these points. The code in the extension of @agoose77 (https://github.com/agoose77/jupyterlab-attachments) adds some nice additional features. Together, both of these resolve this issue. Currently, the hope is to get #5913 in before 1.0, but to wait with the code from the extension. I want to ensure that both of you are OK with this (and that I'm not missing anything important). I guess this question is mostly to @agoose77, as you said you wanted to keep the code of the extension separate from core for a little more.

@vidartf vidartf closed this in #5913 Sep 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants