navossoc / KeePass-Yet-Another-Favicon-Downloader

Yet Another Favicon Downloader for KeePass 2.x
MIT License
532 stars 30 forks source link

Add http:// #5

Closed Qwash closed 6 years ago

Qwash commented 6 years ago

Almost all urls in my database added without "http://" or "https://" at the beggining The plugin should be able to handle it of course without any modification to the url entries in the database

navossoc commented 6 years ago

Sure, I have thought about it when I was writing the plugin.

Do you think it is best to try https:// first, then http://?

Since we are moving to a "HTTPS" period, this maybe make more sense.

Just out of curiosity, why do you enter URLs without a schema? Anyway, let me know.

Qwash commented 6 years ago

HTTPS won't bring any advantages since we are just fetching some icons I think going for HTTP directly is better :)

I don't really remember the reason :D

navossoc commented 6 years ago

Well, I'll take a look on this weekend.

navossoc commented 6 years ago

Hey @Qwash , check this out...

https://github.com/navossoc/KeePass-Yet-Another-Favicon-Downloader/releases/tag/v1.1.0.0-pre

This should do the trick for now, right? Let me know and I will release it as the next update.

Thanks.

Qwash commented 6 years ago

Edit: "This setting is disabled by default." I just noticed this now it's working great now thank you :)

navossoc commented 6 years ago

Ops, you almost got me 😨 for a second.

I will give a few days, then release it as the main version.

Let me know if you find any issue.

Thanks!

navossoc commented 6 years ago

Hey @Qwash, I have promoted that pre-release to a release.

Technically it's the same file. The only modification I did on the code was increment the version. I hope this way, it will notify other users to update.

Qwash commented 6 years ago

OK, thank you 👍

matthewmcpartland commented 6 years ago

Edit: I think it's advantageous to add https:// instead of just http. Part of the workflow of keepass may have a user double click the url and it better protects the user to have them go to https. You could argue http is better for legacy reasons, but an example is https://imdb.com where they don't support ssl encrypted connections yet the browser should still connect the user to the http version instead.

My comment originally suggested not adding http:// to the url but I didn't realize that keepass doesn't support linking to a website without it. It could be nice to keep the http:// adder but with an https option and maybe also have the plugin support the url without http

navossoc commented 6 years ago

@matthewmcpartland have you tested the option yet?

I mean, maybe I am not understanding exactly what are you talking about or I did not make myself very clear about how it works.

It "adds" the prefix http:// on the fly (on memory only). It will not change your password entry on the database.

For example: you have an entry with a URL saved like this: github.com

It will turn it in: http://github.com

This process is done only in memory, the password entry will not be affected. After you perform the "download favicons" action, the URL you have saved in your KeePass database will remain exactly the same.

Is this what you are referring to or did I misunderstand?

About the https, I'll look into too...

matthewmcpartland commented 6 years ago

I thought the option would edit all of the URLs in the database. With the option enabled it works beautifully.

Since I had though it would change the url, then it would be better to have the url be changed to https://. That said since it only loads it into memory I agree you should keep it as just http for legacy support.

Excellent work on this plugin btw, it really picks up where the other one left off

navossoc commented 6 years ago

@matthewmcpartland Yeah, it won't edit anything else besides the custom icon on the password entry.

Excellent work on this plugin btw, it really picks up where the other one left off

That was the plan 👍

I'll change the wiki later to make this behavior more clear and avoid misinterpretation.