-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fixes SecureChannelFailure #21
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Looks like it will be a good improvement to the plugin!
Apart from my line-specific comments, I have a couple of others:
The Microsoft article you cite talks about the need to handle a couple of new exceptions that can be thrown on some systems when using the code that you've submitted. If you're confident that these are already being handled in a sensible way elsewhere in the plugin please put some comments in the code to explain that. Otherwise please can you add some handling for these exceptions?
It's generally good practice to separate whitespace changes from actual code changes in order to make it easier for people to find the actual changes. In this case I don't have a problem with the changes you've proposed (looks like it's essentially fixing up earlier inconsistencies so is a positive thing) so I won't let that stop the pull request being merged this time but if you want to split them out to help me review any subsequent changes that would be cool.
I guess the changes should be fine now. Splitted everything up in a few commits. |
Thanks. LGTM now. I'm holding off the merge for a bit pending the resolution of discussion around the best way to enable me to build and test on Linux (rather than Windows Visual Studio when I last did made a release). See #12 |
This is already been dealt with by my change to use |
I'm not a coder, but when a new release will fix this SecureChannelFailure? No way to download favicons from a lot of websites. Examples: github.com, sourceforge.net (with Windows) That's a great plugin, thanks for it, but the last release is from 2014 and the issue has been widely reported. I don't get it... |
Please see this new post on the KeePass forum: https://sourceforge.net/p/keepass/discussion/329220/thread/9a54032510/ I wanted to post it there to ensure the full range of current and potential users of this plugin can read and respond to it. |
Thank you for the clarification ! I should have searched for alternatives, it was not hard to find... This one works perfectly on Windows: https://github.com/navossoc/KeePass-Yet-Another-Favicon-Downloader |
see Issue #20