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

handle all dropped images #2642

Merged
merged 9 commits into from
Feb 5, 2019
Merged

Conversation

daiyam
Copy link
Contributor

@daiyam daiyam commented Nov 25, 2018

Description

The change allows to drop an image directly from the browser.
It also handles all the images dropped (from a browser or from a file explorer).

Issue fixed

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • 🔘 Improvement (Change that improves the code. Maybe performance or development improvement)
  • ⚪ Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • 🔘 All existing tests have been passed
  • ⚪ I have attached a screenshot/video to visualize my change if possible

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Nov 27, 2018
@daiyam daiyam changed the title drag and drop image from browser handle all dropped images Nov 28, 2018
@daiyam
Copy link
Contributor Author

daiyam commented Nov 28, 2018

I've merged the previous PR #2473 into this one.

@edl7878
Copy link

edl7878 commented Dec 12, 2018

come on, image is a must. but not work on windows for now. frustrated.

@daiyam
Copy link
Contributor Author

daiyam commented Dec 12, 2018

@edl7878 is this PR not working on Windows?

@Rokt33r
Copy link
Member

Rokt33r commented Jan 31, 2019

@daiyam I've checked this pr after merging the current master. It works fine in macOS, but doesn't work in Windows.
The problem is happened when the image url has query. In macOS, it just ignore query when loading image, but, in Windows, it also think query is a filename.

Uncaught Error: ENOENT: no such file or directory, open 'C:\....\hash.jpg?random=query'

Could you fix this and merge master into this pr?
I think we can finally fix pasting image issues after merging this pr.

@daiyam
Copy link
Contributor Author

daiyam commented Jan 31, 2019

@Rokt33r is it from a browser or Explorer?

@Rokt33r
Copy link
Member

Rokt33r commented Jan 31, 2019

@daiyam a browser!

@daiyam
Copy link
Contributor Author

daiyam commented Jan 31, 2019

@Rokt33r I've found the issue. It's in the function copyAttachment(), the method path.extname() is called with the complete url. So with an url like http://..../image.jpg?random=query, it's give the extension.jpg?random=query...

@daiyam
Copy link
Contributor Author

daiyam commented Jan 31, 2019

@Rokt33r your issue should be fixed but you should test it to make sure 😉

Copy link
Contributor

@ehhc ehhc left a comment

Choose a reason for hiding this comment

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

Can you please add tests for the behaviour you want to fix? e.g. for a url containing a query

@Rokt33r Rokt33r added this to the v0.11.14 milestone Feb 1, 2019
@Rokt33r
Copy link
Member

Rokt33r commented Feb 1, 2019

@daiyam Could you add test cases for url with query?

@daiyam
Copy link
Contributor Author

daiyam commented Feb 1, 2019

@Rokt33r @ehhc I've added the tests on urls.

I also had to fix the other tests so they are correctly testing the function.

@Rokt33r
Copy link
Member

Rokt33r commented Feb 1, 2019

@daiyam Awesome! I think I could review this on the next Monday. Please wait a little bit more.
Btw, I've put some money to the issue. So please submit this pr to Issuehunt. https://issuehunt.io/repos/53266139/issues/2630

@Rokt33r
Copy link
Member

Rokt33r commented Feb 4, 2019

@daiyam Ohno... This still does not work well in Linux.

screen shot 2019-02-04 at 12 20 52

Attachment folder ("/home/parallels/Boostnote/attachments/3981a37e-d182-4a95-b5fb-b589853323f3") did not exist..

It seems file extensions are gone somewhere. And I checked the attachments folder but there were nothing.

@daiyam
Copy link
Contributor Author

daiyam commented Feb 4, 2019

@Rokt33r what were you doing? Because the log is normal if there were no images in the note.

@Rokt33r
Copy link
Member

Rokt33r commented Feb 4, 2019

@daiyam I dragged and dropped an image from browser in Ubuntu Linux. Then, nothing was pasted and the only thing I got is that logs....

@daiyam
Copy link
Contributor Author

daiyam commented Feb 4, 2019

@Rokt33r I found an issue with Firefox

In the following screenshot, the first "HTML" is from Firefox and the second is from Chromium.

screenshot

@daiyam
Copy link
Contributor Author

daiyam commented Feb 4, 2019

@Rokt33r I've tested on Ubuntu and it should be fine.

@Rokt33r Rokt33r added approved 👍 Pull request has been approved by sufficient reviewers. and removed awaiting review ❇️ Pull request is awaiting a review. labels Feb 5, 2019
@Rokt33r
Copy link
Member

Rokt33r commented Feb 5, 2019

Awesome! I'll deploy this tomorrow!

@daiyam daiyam deleted the drop-browser-image branch February 7, 2019 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved 👍 Pull request has been approved by sufficient reviewers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants