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

Merge cell attachments when merging cells #8427

Merged
merged 6 commits into from May 20, 2020

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented May 15, 2020

References

Fixes #8414.

There are still a couple of things to check:

Code changes

Extend NotebookActions.mergeCells to also merge attachments for markdown cells.

User-facing changes

Before

merge-cells-attachments

After

merge-cells-attachments-3

Backwards-incompatible changes

None

@jupyterlab-dev-mode
Copy link

jupyterlab-dev-mode bot commented May 15, 2020

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor

jasongrout commented May 15, 2020

  • What about the uniqueness of attachments?

A recent change is to generate UUIDs for attachment keys, so hopefully there won't be collisions!

@jasongrout
Copy link
Contributor

jasongrout commented May 19, 2020

  • Should attachments also be merged for raw cells?

Yes, I think that makes sense.

@jasongrout
Copy link
Contributor

jasongrout commented May 19, 2020

* What about the uniqueness of attachments?

How about for now we pick a winner, rather than trying to parse the text and make sense of where those links are used?

Or we could pop up an error dialog and refuse to merge until the user resolves the attachment conflict.

@jtpio
Copy link
Member Author

jtpio commented May 19, 2020

Or we could pop up an error dialog and refuse to merge until the user resolves the attachment conflict.

It's also possible to select a code cell last, and in that case all attachments will be lost after the merge. If we go with the popup, we'll probably want to add one for that case too.

@jasongrout
Copy link
Contributor

jasongrout commented May 19, 2020

It's also possible to select a code cell last, and in that case all attachments will be lost after the merge. If we go with the popup, we'll probably want to add one for that case too.

Yes, good point. If attachments are deleted, we pop up an error message? Probably the error message would need to list the attachments (and have preview thumbnails?) that would be deleted or overwritten and ask if the user would want to delete/overwrite them?

@jasongrout
Copy link
Contributor

jasongrout commented May 19, 2020

Considering the current situation is that all attachments in other cells are automatically deleted, though, I think this error message dialog and the conflict resolution could be put in another PR if it means that this PR gets in more quickly.

@jtpio
Copy link
Member Author

jtpio commented May 20, 2020

I think this error message dialog and the conflict resolution could be put in another PR

I think so too. The generated UUIDs should already help reduce the number of collisions.

@jasongrout
Copy link
Contributor

jasongrout commented May 20, 2020

@jtpio - I made a PR to this to use the typings more effectively: jtpio#2

@jtpio
Copy link
Member Author

jtpio commented May 20, 2020

Thanks!

(btw feel free to push to this branch directly)

@jtpio jtpio marked this pull request as ready for review May 20, 2020
Copy link
Contributor

@jasongrout jasongrout left a comment

This looks great and works great when I tested it. Thanks!

@jasongrout jasongrout modified the milestones: 2.2, 3.0 May 20, 2020
@blink1073
Copy link
Member

blink1073 commented May 20, 2020

Nice work @jtpio!

@jtpio
Copy link
Member Author

jtpio commented May 20, 2020

Rebasing to include the CI fixes from #8433

@jasongrout jasongrout merged commit 9961cf6 into jupyterlab:master May 20, 2020
39 of 40 checks passed
@jtpio jtpio deleted the merge-attachments branch May 20, 2020
@saulshanabrook saulshanabrook modified the milestones: 3.0, 2.2 Jun 24, 2020
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:notebook status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants