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.

Sign up for GitHub

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

Jump to bottom

Allow spaces in names of files attached to markdown cells #8095

Merged
merged 13 commits into from Mar 30, 2020
Merged

Allow spaces in names of files attached to markdown cells #8095

Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
596193c
attachfiles with spaces in name
ianhi Mar 27, 2020
e5bef35
add URI encoding to lumino events
ianhi Mar 27, 2020
7931e0c
Make URI argument optional
ianhi Mar 27, 2020
81ef6f1
Make URI argument optional
ianhi Mar 27, 2020
37485fd
Use URI if exists, otherwise attachement name
ianhi Mar 27, 2020
f30bac3
Don't overwrite existing attachments
ianhi Mar 27, 2020
c315c2f
Use UUID for URI
ianhi Mar 29, 2020
2123d2e
encode name as valid URI when no URI provided
ianhi Mar 29, 2020
3596154
Stricter mimetype test
ianhi Mar 29, 2020
2622eb6
give generateURI a default argument
ianhi Mar 29, 2020
1ac1803
Always use generateURI
ianhi Mar 29, 2020
ac5df28
Do not encode the attachment name by default
jasongrout Mar 30, 2020
7fc5005
Update packages/cells/src/widget.ts
saulshanabrook Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 25 additions & 10 deletions packages/cells/src/widget.ts
View file
Open in desktop
Expand Up @@ -1169,7 +1169,8 @@ export abstract class AttachmentsCell extends Cell {
* Modify the cell source to include a reference to the attachment.
*/
protected abstract updateCellSourceWithAttachment(
attachmentName: string
attachmentName: string,
URI?: string
): void;

/**
Expand Down Expand Up @@ -1272,20 +1273,21 @@ export abstract class AttachmentsCell extends Cell {
CONTENTS_MIME_RICH
) as DirListing.IContentsThunk;
if (model.type === 'file') {
this.updateCellSourceWithAttachment(model.name);
const URI = this._generateURI(model.name);
this.updateCellSourceWithAttachment(model.name, URI);
Copy link
Contributor

Choose a reason for hiding this comment

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

Below, we now check to see if the data is an image mimetype. Should we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already some mimetype filtering happening in this function when if makes the array of supportedMimetypes

jupyterlab/packages/cells/src/widget.ts

Lines 1245 to 1247 in 5977421

const supportedMimeTypes = toArray(
filter(event.mimeData.types(), mimeType => {
if (mimeType === CONTENTS_MIME_RICH) {

I hoped that this was doing enough filtering. I wasn't able to test this though, because I don't seem to be able to trigger this event. Should this be triggerable by dragging an image from the jupyterlab filebrowser?

void withContent().then(fullModel => {
this.model.attachments.set(fullModel.name, {
this.model.attachments.set(URI, {
[fullModel.mimetype]: fullModel.content
});
});
}
} else {
// Pure mimetype, no useful name to infer
const name = UUID.uuid4();
this.model.attachments.set(name, {
const URI = UUID.uuid4();
ianhi marked this conversation as resolved.
Show resolved Hide resolved
this.model.attachments.set(URI, {
[mimeType]: event.mimeData.getData(mimeType)
});
this.updateCellSourceWithAttachment(name);
this.updateCellSourceWithAttachment(URI, URI);
}
}
}
Expand Down Expand Up @@ -1325,15 +1327,28 @@ export abstract class AttachmentsCell extends Cell {
const mimeType = matches[1];
const encodedData = matches[3];
const bundle: nbformat.IMimeBundle = { [mimeType]: encodedData };
this.model.attachments.set(blob.name, bundle);
this.updateCellSourceWithAttachment(blob.name);
const URI = this._generateURI(blob.name);

if (mimeType.includes('image')){
ianhi marked this conversation as resolved.
Show resolved Hide resolved
this.model.attachments.set(URI, bundle);
this.updateCellSourceWithAttachment(name, URI);
saulshanabrook marked this conversation as resolved.
Show resolved Hide resolved
}
};
reader.onerror = evt => {
console.error(`Failed to attach ${blob.name}` + evt);
};
reader.readAsDataURL(blob);
}

/**
* Generates a unique URI for a file
* while preserving the file extension.
*/
private _generateURI(name: string): string {
const lastIndex = name.lastIndexOf('.');
return lastIndex !== -1 ? UUID.uuid4().concat(name.substring(lastIndex)) : UUID.uuid4();
}
ianhi marked this conversation as resolved.
Show resolved Hide resolved

/**
* The model used by the widget.
*/
Expand Down Expand Up @@ -1445,8 +1460,8 @@ export class MarkdownCell extends AttachmentsCell {
/**
* Modify the cell source to include a reference to the attachment.
*/
protected updateCellSourceWithAttachment(attachmentName: string) {
const textToBeAppended = `![${attachmentName}](attachment:${attachmentName})`;
protected updateCellSourceWithAttachment(attachmentName: string, URI?: string) {
const textToBeAppended = `![${attachmentName}](attachment:${URI ?? encodeURI(attachmentName)})`;
jasongrout marked this conversation as resolved.
Show resolved Hide resolved
this.model.value.insert(this.model.value.text.length, textToBeAppended);
}

Expand Down