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

Attachments are lost after merging two markdown cells #8414

Closed
jtpio opened this issue May 12, 2020 · 7 comments · Fixed by #8427
Closed

Attachments are lost after merging two markdown cells #8414

jtpio opened this issue May 12, 2020 · 7 comments · Fixed by #8427
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@jtpio
Copy link
Member

jtpio commented May 12, 2020

Description

Attachments are lost when two cells are merged with "Edit -> Merge Selected Cells".

Reproduce

Cell 1 contains an attachment (image pasted from the clipboard), for example:

![image.png](attachment:caf34e46-2196-4066-97d8-80e53351c0f0.png)

Cell 2 contains an attachment too, for example:

![image.png](attachment:44a1114f-565f-473a-bbfd-f9244ca93af5.png)

merge-cells-attachments

After the merge, the source of the cell is:

![image.png](attachment:caf34e46-2196-4066-97d8-80e53351c0f0.png)

![image.png](attachment:44a1114f-565f-473a-bbfd-f9244ca93af5.png)

But only one image is displayed.

Expected behavior

It feels like both images should be shown after merging markdown cells. Meaning that the attachments would be merged too if possible (markdown cell).

Looking at the text source of the notebook, the attachments metadata contains only one entry after the merge in the example above.

Context

  • Operating System and version: Ubuntu 20.04
  • Browser and version: Firefox Dev Edition 77.0b4
  • JupyterLab version: master (6d5240d)
@jasongrout
Copy link
Contributor

jasongrout commented May 12, 2020

Indeed, I don't see any code in the merge function handling attachments. This is likely an oversight:

export function mergeCells(notebook: Notebook): void {

@jasongrout jasongrout added this to the Future milestone May 12, 2020
@jasongrout
Copy link
Contributor

jasongrout commented May 12, 2020

Would you like to submit a PR?

@jtpio
Copy link
Member Author

jtpio commented May 13, 2020

Sure, I'll have a look.

@vidartf
Copy link
Member

vidartf commented May 15, 2020

I guess it should only be a dictionary merge, but the interesting thing will be how to handle conflicting keys. If they both have the same value, I guess they can just be merged as is, but if the values differ we might want to find a unique suffix for one of them? Any such rename would then need to update any references in the markdown source as well 🤔

@jtpio
Copy link
Member Author

jtpio commented May 15, 2020

For reference this is what the classic notebook (6.0.3) does:

merge-cells-attachments-notebook

The keys are named image.png. There are also extra commands to copy, cut and paste cell attachments:

image

@jtpio
Copy link
Member Author

jtpio commented May 15, 2020

Opened #8427 as a start.

@jasongrout
Copy link
Contributor

jasongrout commented May 15, 2020

I guess it should only be a dictionary merge, but the interesting thing will be how to handle conflicting keys. If they both have the same value, I guess they can just be merged as is, but if the values differ we might want to find a unique suffix for one of them? Any such rename would then need to update any references in the markdown source as well 🤔

A recent change in JLab is to generate UUIDs for attachments, so hopefully for at least JupyterLab there won't be collisions. Of course, for general ipynb files there isn't that guarantee.

@jasongrout jasongrout modified the milestones: Future, 3.0 May 20, 2020
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jun 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 a pull request may close this issue.

3 participants