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

Add support for dragging attachments from file browser and pasting #5913

Merged
merged 15 commits into from Sep 5, 2019

Conversation

@Madhu94
Copy link
Collaborator

@Madhu94 Madhu94 commented Jan 27, 2019

Will fix #5744

  • Allow dragging files from filebrowser to notebook
  • Allow pasting images into notebook
  • Handle native drop of files
  • Modify markdown cell's source to append the attachment (doing this may need moving of drag and paste event listeners into markdown cell widget, instead of notebook widget?)
  • If a cell attachment is not referenced in any of the cell's outputs, remove that attachment on save
@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Mar 2, 2019

Hi all, this is mostly complete. It would be really helpful if this could be reviewed. Thank you!

@vidartf
Copy link
Member

@vidartf vidartf commented Mar 4, 2019

Is there a way to get the binder-bot to make a link here (or does anybody know how to do it manually)?

@vidartf
Copy link
Member

@vidartf vidartf commented Mar 4, 2019

Copy link
Member

@vidartf vidartf left a comment

Looking at the binder, I see that there is an issue when dragging an image from the file browser: The default behavior of the image being dragged as a tab for viewing is not cancelled when dragging over a cell. It correctly drops the file as an attachment, but the blue box is then stuck on screen until you start a new widget drag operation.

See also specific code comment about trying to coordinate with https://github.com/agoose77/jupyterlab-attachments .

packages/cells/src/widget.ts Outdated Show resolved Hide resolved
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 4, 2019

The default behavior of the image being dragged as a tab for viewing is not cancelled when dragging over a cell. It correctly drops the file as an attachment, but the blue box is then stuck on screen until you start a new widget drag operation.

IIRC, this is because of this phosphor bug: phosphorjs/phosphor#300

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Mar 4, 2019

IIRC, this is because of this phosphor bug: phosphorjs/phosphor#300

@jasongrout Thank you so much, I couldn't figure this out ! This was the reason I added [WIP] to the PR.

It seems like this isn't a trivial fix though? Should we use a UI similar to @agoose77's extension, until we fix the phosphor issue? This extension uses the context menu for lab's filebrowser, not the phosphor drag and drop.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Mar 4, 2019

It seems like this isn't a trivial fix though?

Well, I think the fix is as easy as merging that PR and releasing a phosphor patch update :), but apparently others may disagree? I agree it's not easy to fix outside of phosphor, though perhaps @sccolbert's idea of using multiple mimetypes mentioned in the issue may also be a way to do it. I don't think I tried that, though apparently I thought through it a bit in our conversation several years ago.

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Mar 9, 2019

@jasongrout Multiple mime types are being used here, so I am not sure if I've understood right. application/vnd.phosphor.widget-factory is the mime type that the dockpanel uses, do you mean maybe we can choose to remove it if the drop is handled by a markdown cell widget ? A markdown cell widget would use a different custom mime type (like application/vnd.jupyter.attachments in this PR)

@afshin afshin requested review from tgeorgeux and vidartf Mar 27, 2019
@afshin afshin added this to the 1.0 milestone Mar 27, 2019
Copy link
Member

@vidartf vidartf left a comment

Mostly looking OK, but minor fixes and tweaks. Also, there is still the outstanding issue in the OP about removing attachments when the references are dropped (probably on MD render and/or save/serialization?).

packages/cells/src/widget.ts Outdated Show resolved Hide resolved
packages/cells/src/widget.ts Outdated Show resolved Hide resolved
if (protocol !== 'data:') {
return;
}
const content = href.split(':')[1];
Copy link
Member

@vidartf vidartf Apr 4, 2019

Choose a reason for hiding this comment

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

I think you can use pathname from URLExt.parse instead :)

Copy link
Collaborator Author

@Madhu94 Madhu94 Jun 19, 2019

Choose a reason for hiding this comment

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

This had an issue with firefox where pathname came out to be '', I'm not sure why

packages/cells/src/widget.ts Outdated Show resolved Hide resolved
packages/cells/src/widget.ts Outdated Show resolved Hide resolved
@vidartf
Copy link
Member

@vidartf vidartf commented Apr 4, 2019

Also: When testing on Binder with Chrome, I couldn't get pasting of files from the system clipboard to work (pasting was greyed out in the system context menu, and Ctrl+V didn't do anything). Is this expected?

@agoose77
Copy link
Contributor

@agoose77 agoose77 commented Apr 5, 2019

@Madhu94 I'm wondering, does the 'drop non-referenced attachments' feature exist in classic notebook? If not, could this lead to UX confusion for users?

@vidartf
Copy link
Member

@vidartf vidartf commented Apr 29, 2019

@Madhu94 Madhu94 force-pushed the add-attachments branch 2 times, most recently from ad5faf6 to d8d4525 Jun 1, 2019
@blink1073 blink1073 removed this from the 1.0 milestone Jun 9, 2019
@blink1073 blink1073 added this to the 1.1 milestone Jun 9, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 14, 2019

@fperez mentions that this might be considered a 1.0 issue, in that it is a missing feature from the classic notebook.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 14, 2019

Counterpoint: this feature didn't exist in notebook when we made a 1.0 milestone.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 14, 2019

@Madhu94 - how close do you think this is to being finished?

@Madhu94 Madhu94 force-pushed the add-attachments branch from d8d4525 to 9ecee8d Jun 19, 2019
@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Jun 19, 2019

[Apologies for my long absence from Github]

I took care of the first set of review comments, pending work is removing unreferenced attachments (on it now) and the phosphor issue mentioned in this comment (IMO, more of a usability issue than unreferenced attachments)

Please add any other review comments, if needed

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Jun 19, 2019

Also: When testing on Binder with Chrome, I couldn't get pasting of files from the system clipboard to work (pasting was greyed out in the system context menu, and Ctrl+V didn't do anything). Is this expected?

Strange. Can you tell me how you tested ? I opened random images from Google images, clicked 'Copy Image' from the right-click context menu and then did Ctrl+V after focusing on the markdown cell in the notebook.

@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Sep 4, 2019

Potentially, if we are being strict about things,

@vidartf Just for my curiosity, could you please explain what issues you foresee? In case I have to upgrade stuff again, in the future

Madhu94 and others added 14 commits Sep 4, 2019
files from filebrowser, native drag/drop or by pasting
@phosphor/widgets@1.9.0 has a bug fix - dockpanel overlay
did not hide itself even if markdown cell handled the p-drop event.
Change from a custom attachments mimetype to
mimetype of the contents model with the suffix
";closure=true"
Adds a rich contents mime type to allow extensions to more directly use the already fetched content models of the filebrowser. This is then used in the attachment cell widgets to synchronously insert the name (which will initially resolve to a broken image), and then start the async fetch.

Also reefactors `updateCellSourceWithAttachment`.
@vidartf vidartf force-pushed the add-attachments branch from fd1d764 to cea5098 Sep 4, 2019
@vidartf vidartf force-pushed the add-attachments branch from cea5098 to b874b3a Sep 4, 2019
vidartf
vidartf approved these changes Sep 4, 2019
@vidartf vidartf merged commit 8cd72f7 into jupyterlab:master Sep 5, 2019
9 checks passed
@Madhu94
Copy link
Collaborator Author

@Madhu94 Madhu94 commented Sep 5, 2019

Whoo ! Finally ! :) Thanks for your help @vidartf .

@vidartf
Copy link
Member

@vidartf vidartf commented Sep 5, 2019

Thanks for you efforts and your patience @Madhu94 :)

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Sep 5, 2019

Thanks for the incredible work and patience @Madhu94!

@jasongrout jasongrout removed this from the 2.0 milestone Sep 10, 2019
@jasongrout jasongrout added this to the 1.2 milestone Sep 10, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 28, 2019

Did this also sove #7085?

@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants