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

Always use 'image' as name for pasted images #13339

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Oct 28, 2020

Some browsers seem to put file system paths into blob.name which look rather odd and should not be part of the posted content. Always set name to static "image" like GH does.

Ref: #13333 (comment)

@a1012112796 please confirm this works for you, also test multiple pastes.

Some browsers seem to put file system paths into blob.name which look
rather odd and should not be part of the posted content. Always set name
to static "image" like GH does.
@silverwind silverwind changed the title always use 'image' as name for pasted images Always use 'image' as name for pasted images Oct 28, 2020
@silverwind
Copy link
Member Author

silverwind commented Oct 28, 2020

I generally wonder if it's ever desirable to accept the browser-provided filename. Generally I think not because it's bound to always be something autogenerated because clipboard conceptionally stores a byte stream without a filename, thought there might be exceptions.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 28, 2020
@techknowlogick techknowlogick added this to the 1.14.0 milestone Oct 28, 2020
@silverwind
Copy link
Member Author

Here's something to check out browser behaviour. On macOS, I see get image.png in Firefox, Chrome and Safari. I wonder if there are differences on other platforms.

@zeripath
Copy link
Contributor

zeripath commented Oct 28, 2020

Couldn't we look for the last index of '/' and trim between it and the last indexof '.'? (defaulting to 'image' if that is nothing)

@silverwind
Copy link
Member Author

silverwind commented Oct 28, 2020

Looking at #13333 (comment) again, it appears the strange path appears before the pasted content.

So I think there might be two dataTransfer items (one text, one image) and it ends up pasting both but [image] is still visible in the gif so at least the image's filename seems consistent with my tests. We could just ignore any textual items when an image is contained to work around.

@a1012112796 please tell the exact os/browser used there and if it happens on CTRL-V too as opposed to right click. Also, what tool is used to copy the content and could it be that that tool puts two items on the system clipboard?

@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Oct 29, 2020
@a1012112796
Copy link
Member

@silverwind Thanks for your work, But sadly it maybe not usefull.

test result:
tmp1

test with ctrl + v:
tmp2

test on gh:
tmp4

test with a simple text editor, you can see it only add one link to system clipboard, not other thing.
tmp3

test with ubunt v20.04, chrome 86.0.4240.111(正式版本) (64 位)

@silverwind
Copy link
Member Author

@a1012112796 can you try pasting on https://jsfiddle.net/silverwind/acezbfkd/?

It should only show the image item like this:

image

@a1012112796
Copy link
Member

a1012112796 commented Oct 29, 2020

@a1012112796 can you try pasting on https://jsfiddle.net/silverwind/acezbfkd/?

It should only show the image item like this:

image

Hmm, I see.

image
let me have a look on the code.

uploadFile(img, (res) => {
const data = JSON.parse(res);
replaceAndKeepCursor(field, `![${name}]()`, `![${name}](${AppSubUrl}/attachments/${data.uuid})`);
replaceAndKeepCursor(field, `![image]()`, `![image](${AppSubUrl}/attachments/${data.uuid})`);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
replaceAndKeepCursor(field, `![image]()`, `![image](${AppSubUrl}/attachments/${data.uuid})`);
replaceAndKeepCursor(field, `![Uploading image ...]()`, `![image](${AppSubUrl}/attachments/${data.uuid})`);

@@ -352,11 +352,10 @@ function initImagePaste(target) {
const field = this;
field.addEventListener('paste', (event) => {
retrieveImageFromClipboardAsBlob(event, (img) => {
const name = img.name.substr(0, img.name.lastIndexOf('.'));
insertAtCursor(field, `![${name}]()`);
insertAtCursor(field, `![image]()`);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
insertAtCursor(field, `![image]()`);
insertAtCursor(field, `![Uploading image ...]()`);

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is helping any, I prefer the text to not change after upload.

@a1012112796
Copy link
Member

a1012112796 commented Oct 29, 2020

suggest show error message on the editor bottom when uploading image failed like Upload image failed, and remov previous added strings.

@silverwind
Copy link
Member Author

silverwind commented Oct 29, 2020

The issue is your paste actions pastes two things, a path and a image while generally the expectation is that only a image is there. I think we can workaround by ignoring textual items when a image is present. I'm not sure how GitHub handles this as the minified JS is quite hard to read but I assume you don't have the issue on GitHub, right?

Maybe there is a helper JS library around to handle image paste for us.

@silverwind silverwind closed this Oct 29, 2020
@silverwind
Copy link
Member Author

Closing as this approach is wrong, may check it out later in a Ubuntu VM.

@silverwind silverwind deleted the paste-name branch October 29, 2020 19:59
@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants