PactInteractive / image-downloader

Download images from the web more easily. A browser extension for Google Chrome, Microsoft Edge, and Brave.
https://chrome.google.com/webstore/detail/image-downloader/cnpniohnfphhjihaiiggeabnkjhpaldj
809 stars 298 forks source link

Supported file renaming #20

Closed kitlawes closed 7 years ago

kitlawes commented 7 years ago

This pull request is intended to solve issue #5. The feature for renaming files is disabled by default.

kitlawes commented 7 years ago

Let me know if there's anything I can do to improve this pull request - many thanks

vdsabev commented 7 years ago

Thanks for taking the time to write a PR! I apologize for the delay, I skimped over the code, but haven't had the chance to test it yet. I'll do that next week 😉

kitlawes commented 7 years ago

Thanks - let me know if I can help at all :+1:

vdsabev commented 7 years ago

@kitlawes apart from some styling and copywriting, everything looks good.

I tested the changes you made, and noticed the following use case:

Steps

  1. Select files a.png, b.png, c.png, d.gif, e.jpg, f.jpg
  2. Set the rename to z
  3. Download

Actual result

On Windows, you get z.png, z (1).png, z (2).png, z.gif, z.jpg, z (1).jpg

Alternative

I think most people would actually expect to get z1.png, z2.png, z3.png, z4.gif, z5.jpg, z6.jpg. This could be implemented using a counter. What do you think about that behavior?

vdsabev commented 7 years ago

If you think the alternative renaming I proposed is useful, but have no time or desire to work on it, I think you can give me permission to make changes to this PR: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

I can then also update the manifest version and the changelog in the readme.

kitlawes commented 7 years ago

Thank you for the review @vdsabev and for taking the time to test. I have implemented your suggestion about the filename numbers. Also, you already have permission to make changes to this pull request (I think by default) - the 'Allow edits from maintainers.' checkbox is ticked. Feel free to make any changes, and let me know if there are any issues with permissions anyway.

vdsabev commented 7 years ago

@kitlawes please take a look at the changes I made, and I would also really appreciate if you test the extension manually. It seems to work well for me, but I'd feel better with a second opinion.

kitlawes commented 7 years ago

Thank you for these changes. I did a quick test with all combinations of:

Everything worked as expected. The only thing I noticed is that images of different file types are given different names (e.g. a1.png and a2.gif), but they can be named the same (e.g. a.png and a.gif). Is this the behaviour we want? My opinion is that it is fine as is.

vdsabev commented 7 years ago

Yes, I would assume files with different extensions should be named a1.png and a2.gif, because the image extension is sometimes completely arbitrary. I think people are more likely to use the feature that way, but it might turn out I'm wrong after getting some feedback.

Thank you so much for contributing to the project! You get a special medal 🥇 😆 I'll merge this PR and publish later today 😉

kitlawes commented 7 years ago

Great - thanks! :+1:

vinixwu commented 7 years ago

I installed this today, and tried to download from a forum page with images from imgur.com. I typed "img" in rename box, but these files were downloaded with their original names, still.

vdsabev commented 7 years ago

@vinixwu can you tell me the specific URL of the forum post where the rename failed?

Without knowing which forum page you're talking about, I couldn't reproduce the issue you describe. I opened imgur.com and downloaded all images on the front page:

before

And then the folder contained all images, named img1 - img64, as expected:

after

vinixwu commented 7 years ago

It's a forum in Traditional Chinese. It needs login to see the images. But it is filled with embedded images from imgur.com.

The web site address is: 159.eyny.com.