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

Desktop: Fixes #6211: Try to replace the external link with internal link when attachment file is pasted in Markdown editor #6865

Merged
merged 6 commits into from
Jan 11, 2023

Conversation

wh201906
Copy link
Contributor

@wh201906 wh201906 commented Sep 19, 2022

This will fix #6211. When the user copies an image from WYSIWYG editor, the clipboard will hold the absolute path of the image. The bug is when pasting it to Markdown editor, the absolute path will be pasted. Codes in this PR will try to replace the absolute path with the internal link if possible, just like copying and pasting an image in WYSIWYG editor.
I tested it on Ubuntu and the bug is fixed.
I have no idea about how to call an async function in an sync function. I just imitated the existing code. Please tell me if my code is wrong or not appropriate and I will fix it.

@wh201906
Copy link
Contributor Author

wh201906 commented Sep 19, 2022

I think editorPasteText() should run editorRef.current.replaceSelection(clipboard.readText()) if the replaceResourceExternalToInternalLinks() returns an empty string or "promise gets rejected". But I don't know how to implement this.
The async function replaceResourceExternalToInternalLinks(body, options) just replace the specified text in body, so there is no need to add extra code to handle the empty result. Also, this function doesn't throw errors, and it doesn't call other async functions, so the promise is always resolved.

@wh201906

This comment was marked as resolved.

@laurent22
Copy link
Owner

We need a clear description of what's been fixed, and a working pr.

@wh201906 wh201906 changed the title Desktop: Fixes #6211: Try to replace the external link with internal link Desktop: Fixes #6211: Try to replace the external link with internal link when attachment file is pasted in Markdown editor Sep 19, 2022
@wh201906
Copy link
Contributor Author

wh201906 commented Sep 19, 2022

We need a clear description of what's been fixed, and a working pr.

I have updated the title and description of this PR. Are they correct?
I think the PR is working now since it passed all CI test, and I have tested the functionality of it on Ubuntu and it works as expected.

Comment on lines 297 to 300
void Note.replaceResourceExternalToInternalLinks(clipboard.readText(), { useAbsolutePaths: true })
.then((modifiedMd) => {
editorRef.current.replaceSelection(modifiedMd);
});
Copy link
Owner

Choose a reason for hiding this comment

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

Don't use .then(), use async/await

Copy link
Contributor Author

@wh201906 wh201906 Sep 30, 2022

Choose a reason for hiding this comment

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

I don't know if my code in d9d8f60 is right. It passed the CI test, then I tested it on Ubuntu and it works as expected.

@wh201906
Copy link
Contributor Author

wh201906 commented Oct 1, 2022

@laurent22 Hi. Is there anything I need to do?

@laurent22
Copy link
Owner

Thanks for fixing this, and I think that's the right fix, but I wonder if we want to support this kind of copy and paste between wysiwyg and markdown editor? Lots of other things probably don't work, such as copying tables, and it feels like the solution is simply to not do it.

Why would someone want to copy from one editor to another anyway? Why not stay on the same one?

@wh201906
Copy link
Contributor Author

wh201906 commented Oct 16, 2022

but I wonder if we want to support this kind of copy and paste between wysiwyg and markdown editor?

I guess this PR can be applied to more situations. Any external links referring to something in profile(resources) directory can be converted into the internal links.

Lots of other things probably don't work, such as copying tables

I don't know why these things won't work after merging this PR. Could you please give me more information or examples?
Plus, this PR is based on the behaviors of TinyMCE editor, as I mentioned in this comment. If something doesn't work there, the TinyMCE editor should have the similar problem.

@wh201906
Copy link
Contributor Author

wh201906 commented Oct 16, 2022

Another solution is to find the resource parts in the clipboard which look like [...](...), then replace them only.

@tomasz1986
Copy link

tomasz1986 commented Oct 16, 2022

Why would someone want to copy from one editor to another anyway? Why not stay on the same one?

I mainly use WYSIWYG, but I also switch between the two when I want to add formatting that isn't available in the top bar (e.g. H4/5/6 or straight-up HTML code), and if I happen to have an image that I want to insert copied to clipboard already, I may as well paste it in the Markdown editor as well.

The main problem here is probably that the non-technical user isn't really aware that such a link is absolute and device specific. With tables and other complex formatting, the user can immediately see when something isn't right. With these images, it's only after they've synced their data to another machine when they discover that the links are broken. In the worst case scenario, at that point they may not even have the original images anymore.

@wh201906
Copy link
Contributor Author

wh201906 commented Nov 6, 2022

I think this problem is easier to be triggered than we expected.
https://discourse.joplinapp.org/t/why-is-github-issues-long-left-to-be-answered/25993
https://discourse.joplinapp.org/t/disappearing-accessories-a-serious-functional-defect/28082
I'm willing to renovate this PR if the current version would cause any problem.

@tomasz1986
Copy link

tomasz1986 commented Nov 6, 2022

I think this problem is easier to be triggered than we expected.

Yeah, and I think another aspect which hasn't been pointed enough yet is that this issue literally causes the user to lose data (e.g. when they no longer have access to the original device and/or file). The user who has lost data due to a bug is usually a very unhappy user.

@laurent22
Copy link
Owner

recheck

@github-actions
Copy link
Contributor

github-actions bot commented Dec 21, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@wh201906
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@laurent22
Copy link
Owner

Ok let's merge, this change seems relatively safe and still an improvement. Thanks for implementing this @wh201906!

@laurent22 laurent22 merged commit 3bee0a1 into laurent22:dev Jan 11, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2023
@wh201906 wh201906 deleted the md-absolute-path branch January 12, 2023 00:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joplin saves resources with absolute Windows Paths causing broken synced notes and .jex file is missing images
3 participants