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

Add favicon fetch button next to entry's url edit textbox #2439

Conversation

Projects
None yet
2 participants
@kneitinger
Copy link
Contributor

kneitinger commented Oct 31, 2018

Description

When WITH_XC_NETWORKING is defined, create a QToolButton beside
the Edit Entry -> Entry -> URL which when pressed, acts as though the
Edit Entry -> Icon -> Download Favicon button is pressed. This button
is disabled (grayed-out) when the URL text is empty, and enabled when
the text is present. This PR also removes the UrlFetchProgessDialog class
in favor of a "Custom icon successfully download" message (if triggered from
the edit entry tab).

Motivation and context

Fixes #936

How has this been tested?

  • Compile with -DWITH_XC_NETWORKING=OFF
    • Start to create new entry "Favicon Fetch Entry", verify no button present next to url box
      2018-10-30_22-58-00
    • Enter URL in text box, verify still no button
  • Compile with -DWITH_XC_ALL=ON
    • Start to create new entry "Favicon Fetch Entry (Networking Enabled)", verify icon button is present, yet disabled
      2018-11-18_19-25-36_no-url
    • Enter text in the URL box and verify the button becomes clickable
      2018-11-18_19-26-30_with-url
    • Click button. Verify progress dialogs appear and no error is present. (removed in this PR). Verify "Custom icon successfully downloaded" message appears.
      msg
    • Go to Icon tab and verify presence of correct favicon
      2018-10-30_23-08-33
    • Return to main entry edit tab and click button again, verify that "Custom icon already exist" info message appears
      already
    • Change URL to nonexistent URL and attempt to fetch icon for it. Verify "Unable to fetch favicon" error message appears, and last correct favicon still set.
      2018-10-30_23-08-00
    • Save entry and verify that it has new icon in database widget
      2018-10-30_23-09-21
  • Run existing tests both with and without networking.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Oct 31, 2018

There are a few things I'd like to get input on:

  • Should there be an option in the application settings to disable this? -DWITH_XC_NETWORKING=OFF will disable it, but perhaps there are folks using a prebuilt binary that may not want this feature.
  • Is the button's icon a good choice? It is the same one as the Edit Entry -> Icon tab, but might not be the best choice for this small button
@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Oct 31, 2018

I was playing around with this some more, and I found a bug with the existing favicon fetcher (tested on my branch, current develop, and 2.3.4).

When I:

  • Create a new entry with a valid URL
  • click "Download favicon" in Icon page
  • Save entry and (not necessarily immediately) close keepassxc
  • Reopen keepassxc and edit the same entry
  • click "Download favicon" in Icon page

it fetches another occurrence of the same favicon instead of displaying the blue "Custom icon already exist" message.
2018-10-31_00-54-04
This can be repeated any number of times. If someone could take a moment and reproduce this behavior, I'd like to include a fix in this PR.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 31, 2018

The duplicate icon download is usually because the icons are actually different. I fixed a bug a while back where the comparison between icons was occurring prior to resize, it now does it after resize which produces better results (not perfect though).

I recommend a download image button like this: image-download-symbol

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 17, 2018

@kneitinger are you going to update the icon to something a little more adequate?

@droidmonkey droidmonkey added this to the v2.4.0 milestone Nov 17, 2018

@droidmonkey droidmonkey added the ux label Nov 17, 2018

@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Nov 17, 2018

@droidmonkey yep, looks like I can copy and modify one of the default entry icons to look like yours but still fit in with the rest of the UI icons.
2018-11-17_13-26-14_faviconfetchpre

I noticed that the favicon fetch progress bar is still having issues closing in cases where it finds the icon without having to fallback to duckduckgo (for instance when the URL is 'https://reddit.com'). I put in a fix that destroys it explicitly as soon as it knows it failed, but I'm wondering if we should just get rid of the progress bars? The files are so small that even on slower connections I imagine they are either at 0% or 100%. On even moderately fast connections the progress bars (when acting correctly) disappear before they could even be seen. What do you think?

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 17, 2018

I totally agree with eliminating the progress bars, they have been a pain in the butt since they were put in. I also can no longer download any icons... so there is something fishy going on.

Looks like I am having SSL errors. Might be something to do with testing snapshot builds.

@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Nov 17, 2018

Can't download icons in what situations? Fallback disabled? I don't seem to have problems with them, but if you can give me an example URL that is not working for you, I'm happy to check some recent changes for anything that would cause that.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 17, 2018

Yah the error I am getting is "Cannot create SSL context". This is on the latest develop running right from the build tool. Also cannot download from the snapshot builds available at https://snapshot.keepassxc.org.

@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Nov 18, 2018

Oh the reason I hadn't run into it yet (despite current dev being merged into my branch) is whether I paste from the URL from the browser or enter the URL by hand I always have 'https://' in the URL. So this seems to only affect URLs without that. Putting a leading https:// works on the appimage.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 18, 2018

This is a Windows-Only issue, I figured it out. You need to install OpenSSL on your system (for now): https://slproweb.com/products/Win32OpenSSL.html

We need to separately figured out how to package the needed libraries.

@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Nov 18, 2018

Not sure its working completely correctly on linux either. I don't recall ever seeing this message in the console upon startup:

qt.network.ssl: QSslSocket: cannot resolve SSLv3_client_method
qt.network.ssl: QSslSocket: cannot resolve SSLv3_server_method

And any urls without a leading 'https://' fail to download.

kneitinger added a commit to kneitinger/keepassxc that referenced this pull request Nov 19, 2018

Create new icon for entry tab favicon download button
Per feedback on keepassxreboot#2439, change favicon download button from the
app/preferences-desktop-icons image used by the Icon tab, to a new
image of an picture icon next to a downward facing download arrow

kneitinger added a commit to kneitinger/keepassxc that referenced this pull request Nov 19, 2018

Remove UrlFetchProgressDialog class
Per conversation in keepassxreboot#2439, remove the progress dialog that appears when
downloading an entry's URL's favicon since (when working correctly) it
disappears before it can be read. When downloading icons from the button
located next to the URL text box, display a message panel that confirms
the download was a success.

@droidmonkey droidmonkey requested review from droidmonkey and phoerious Nov 19, 2018

@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Nov 19, 2018

@droidmonkey I see activity from you right now. I want to revise the PR body to reflect the changes accurately, and fix one UI issue I just caught. I'll post when these are taken care of (~20 mins).

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 19, 2018

No rush, I am just trolling. I am also compiling your branch right now to check it out.

Edit: Works great, love it!

Aside from this PR it might be nice to have the entry icon visible on the top of the edit entry view. This would be similar to the entry preview panel.

kneitinger added a commit to kneitinger/keepassxc that referenced this pull request Nov 19, 2018

Do not show successful icon download msg if icon alread exists
In cases where the custom icon already exists and the entry tab's
icon download button is pressed, prevent the behavior where the
"Custom icon successfully downloaded" message is displayed before
displaying the "Custom icon already exists" message.

PR: keepassxreboot#2439
@kneitinger

This comment has been minimized.

Copy link
Contributor Author

kneitinger commented Nov 19, 2018

Fixed issue and updated PR body.

In case it is ever needed, here is the raw .svg for the icon (gzipped to make github file uploader happy)

favicon_download.svg.gz

favicon_download.svg.gz (corrected shadow direction)

kneitinger added a commit to kneitinger/keepassxc that referenced this pull request Nov 19, 2018

Correct light direction on favicon-download arrow's shadow
Lighting made little to no sense considering author simply reflected
an up arrow svg through the x-axis, including it's shadow

PR keepassxreboot#2439
@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 19, 2018

We'll bring this in once the big refactor PR's are merged

kneitinger added some commits Oct 31, 2018

Add favicon fetch button next to entry's url edit textbox
When WITH_XC_NETWORKING is defined, create a QToolButton beside
the Edit Entry -> Entry -> URL, which when pressed, acts as though the
Edit Entry -> Icon -> Download Favicon button is pressed.  This button
is disabled (grayed-out) when the URL text is empty, and enabled when
the text is present.

Fixes #936
Create new icon for entry tab favicon download button
Per feedback on #2439, change favicon download button from the
app/preferences-desktop-icons image used by the Icon tab, to a new
image of an picture icon next to a downward facing download arrow
Remove UrlFetchProgressDialog class
Per conversation in #2439, remove the progress dialog that appears when
downloading an entry's URL's favicon since (when working correctly) it
disappears before it can be read. When downloading icons from the button
located next to the URL text box, display a message panel that confirms
the download was a success.
Do not show successful icon download msg if icon alread exists
In cases where the custom icon already exists and the entry tab's
icon download button is pressed, prevent the behavior where the
"Custom icon successfully downloaded" message is displayed before
displaying the "Custom icon already exists" message.

PR: #2439
Correct light direction on favicon-download arrow's shadow
Lighting made little to no sense considering author simply reflected
an up arrow svg through the x-axis, including it's shadow

PR #2439

@droidmonkey droidmonkey force-pushed the kneitinger:feature/favicon_fetch_button_beside_url_box branch from bd7cd8f to 94f15ab Nov 24, 2018

@droidmonkey droidmonkey merged commit a90a577 into keepassxreboot:develop Nov 24, 2018

4 checks passed

CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

kneitinger added a commit to kneitinger/keepassxc that referenced this pull request Mar 17, 2019

Remediate errors in various favicon fetch scenarios (keepassxreboot#2779
)

Fixes stuck "Download favicon" button on icon download attempts for IP
address hosts by skipping attempts to get 2nd level domain resources
(which resulted in calls to 0.0.0.<rightmost octet of original IP>).

Fixes some cases when DuckDuckGo fallback fails to find icon of >2-level
domains, by adding a request to a DDG URL based on entry's 2nd level
domain.

Repurposes EditWidgetIcons' private fetchCanceled slot (which as of keepassxreboot#2439,
is unused by any code) into public abortRequests slot, which is
connected to the entry edit widget's accepted and rejected signals (in
other words, Ok or Cancel was pressed).

droidmonkey added a commit to kneitinger/keepassxc that referenced this pull request Mar 17, 2019

Remediate errors in various favicon fetch scenarios (keepassxreboot#2779
)

Fixes stuck "Download favicon" button on icon download attempts for IP
address hosts by skipping attempts to get 2nd level domain resources
(which resulted in calls to 0.0.0.<rightmost octet of original IP>).

Fixes some cases when DuckDuckGo fallback fails to find icon of >2-level
domains, by adding a request to a DDG URL based on entry's 2nd level
domain.

Repurposes EditWidgetIcons' private fetchCanceled slot (which as of keepassxreboot#2439,
is unused by any code) into public abortRequests slot, which is
connected to the entry edit widget's accepted and rejected signals (in
other words, Ok or Cancel was pressed).

droidmonkey added a commit that referenced this pull request Mar 17, 2019

Remediate errors in various favicon fetch scenarios (#2779)
Fixes stuck "Download favicon" button on icon download attempts for IP
address hosts by skipping attempts to get 2nd level domain resources
(which resulted in calls to 0.0.0.<rightmost octet of original IP>).

Fixes some cases when DuckDuckGo fallback fails to find icon of >2-level
domains, by adding a request to a DDG URL based on entry's 2nd level
domain.

Repurposes EditWidgetIcons' private fetchCanceled slot (which as of #2439,
is unused by any code) into public abortRequests slot, which is
connected to the entry edit widget's accepted and rejected signals (in
other words, Ok or Cancel was pressed).

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.