Download all favicons #3169

varjolintu · 2019-05-21T07:05:09Z

Adds a new feature for downloading all favicons.

Type of change

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

Description and Context

The following features have been added:

Group context menu item "Download all favicons"
Each entry has a context menu item "Download favicon". Multiple selections are allowed.
Separate class for downloading icons
Separate dialog class for downloading multiple favicons simultaneously (with status for each favicon)
Extra setting for download timeout

Screenshots

Not a screenshot, but a video demonstration of the features:
https://www.dropbox.com/s/cwy1s249hkp4aix/dafi.mp4

Testing strategy

Manually.

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]

rugk · 2019-05-21T07:27:51Z

Looks great in the video! 😆 🎉

One question, however: How does it handle duplicate icons?

e.g., if I have multiple entries for the same website
if multiple domains use the same favicon
if I have already downloaded the latest favicon (manually, with previous versions)

IMHO, it should always "deduplicate" them, i.e. if e.g. the hash is the same, do not insert it as a new icon.

varjolintu · 2019-05-21T07:31:23Z

@rugk It doesn't add duplicate icons to the database. At this point it doesn't load favicons for entries that have a custom icon already set.

rugk · 2019-05-21T07:31:40Z

Also another question: Is there some setting/feature to also download these favicons automatically, when a new entry is added, i.e. something like keepassxreboot/keepassxc-browser#86?

varjolintu · 2019-05-21T07:34:09Z

@rugk That would be a nice addition, as an extra setting of course :)

tomhundt · 2019-05-21T15:23:38Z

Hey, I'm psyched to see the "Download favicon" on a single entry...

Not sure a "Download all" (on a group) is needed -- couldn't one accomplish the same thing (UI-wise) by doing

hit Tab to move focus to the entries side
select all the entries using Cmd-A (Ctrl-A)
right-clicking > "Download favicon" (or hitting Cmd-Shift-D, as you have it set up)

If one really wanted to do this without having to move focus, perhaps there could be a menu item "Entries > Select all entries" (bound to Cmd-A)? Note that selecting more than one group is not allowed, so, focus being on the Group side would not be a problem. (If there's any chance this will change, we should not do it, though. Or bind it to something that doesn't conflict, like Cmd-Shift-A.)

BTW the use case of "I want to set ALL my entries' favicons" could be accomplished by doing a search for something that appears everywhere, like "http" or a space. This effectively gives you a result set of "everything", upon which you can then do "Select all" and then "Download favicon". Boom, done.

Of course, in my case, I'd have to figure out how to do "Remove favicon" for most of them, first... ;-)

IF "Download favicon" instead of doing that action were to bring up an "Icon" dialog similar to the current one in "Edit entry", and IF that dialog had a button to download the icon (currently it can only set custom ones) and IF it also had a "Remove" (or Replace) icon... then my life would be golden, in this regard ;-)

droidmonkey · 2019-05-21T15:27:23Z

@tomhundt the whole point of this feature is to apply icons to "all" entries. Doing it per-entry and per-group is very smart and the right approach for everyone.

varjolintu · 2019-05-21T16:16:32Z

I don't want to add every possible feature to this PR anyway. Let's keep it simple and add things later.

src/gui/DownloadIcon.cpp

src/gui/IconDownloaderDialog.cpp

src/gui/MainWindow.cpp

src/gui/IconDownloaderDialog.cpp

varjolintu · 2019-05-24T06:19:58Z

Last fix: I moved the timeout setting to General settings page.

droidmonkey

Didn't review everything, these are pretty fundamental and need fixing

src/CMakeLists.txt

src/gui/DatabaseWidget.cpp

varjolintu · 2019-05-24T15:33:56Z

@droidmonkey The changes you suggested are now made.

DJCrashdummy · 2019-06-14T13:15:04Z

@vraravam as you can read at #3169 (comment), not now (resp. in this PR).
such a feature needs a little bit more work "behind the scenes" to be as good as it should be, like suggested at #3169 (comment) (catchword: deduplication). - i would not like to have all entries changed every time i "force download all icons" just because i want to check/update if there are new ones... because sooner or later the complete history will be full with identical entries and completely pointless! ☹️
--> so a simple "force download and replace all icons" won't do the trick! ...not to mention that a kind of an (optional) cleanup for no longer used icons in the database would also be needed.

@varjolintu also one thing about download single icons: if someone uses the context-menu to download the icon for one particular entry, can't/shouldn't that be considered as "force download", no matter if the entry already uses a custom icon or not? i'm just thinking loud about the workflow and UX. 🤔
...but for sure - as already mentioned - the entry should just be changed if there is a different icon downloaded!

or wouldn't it be considered as a "shortcut" to the button in the "edit entry"-menu in general? 🤔

varjolintu · 2019-06-14T13:24:57Z

@DJCrashdummy You are exactly right with the points if a force download would be implemented. It's not as simple after all.

For now the single entry download (which can be used with multiple selections too) is not forced either. It could be easier to implement, but I think I'll leave it out from this PR.

DJCrashdummy · 2019-06-14T18:02:13Z

@varjolintu i see your point keeping just the basic function in this PR. 👍
i was just thinking loud about further improvement... because if just this basic function will make it into a release, one thing i'm 100% sure is, that users will complain and fill bugreports, that "download all/selected icons" does not work, because users hardly distinguish between default/custom icons.

droidmonkey

Still reviewing, wanted to get this comment out.

src/gui/DatabaseWidget.cpp

droidmonkey · 2019-06-27T21:35:45Z

Whew! I basically changed @varjolintu entire code submission. The changes move functionality into their respective classes and introduce a global QNetworkAccessManager instance. This streamlines everything and eliminates thread handling in the display dialog. Some functionality differences from the original PR:

Selecting one or more entries to download icons always forces the download (ie, if a new URL exists the new icon will be downloaded and set)
Instead of downloading for each entry, the web url's are scraped from the provided entries and only those urls are downloaded. The icon is set for all entries that share a URL. This is useful if a group contains many entries that point to the same url, only 1 download call will occur.
The icon download dialog displays whether you are doing one entry, many entries, or an entire group. It is also modal so you have to dismiss it to use KeePassXC again.
The timeout setting is always enabled.
I moved the DuckDuckGo fallback notice into the download dialog.

Some screenshots:

varjolintu · 2019-06-28T05:05:44Z

Looks great! While I was testing it, it seems that if I try to abort the transfer by closing the dialog right away its shown, the whole KeePassXC gets stuck until all icons are downloaded (one entry in my test database is a URL that exists but the site is not available at all, so it timeouts every time).

droidmonkey · 2019-06-28T14:55:48Z

Cancel on close seems to work for me. When there are a ton of entries it takes a relatively long time to processing the list. I am not sure what takes so long, but I suspect it is the resolving of the "webURL" which may highlight some optimizations we should make to the DB in general.

One thing I noticed, we should not show the action on the recycle bin or entries in the recycle bin.

* Selecting one or more entries to download icons always forces the download (ie, if a new URL exists the new icon will be downloaded and set) * Instead of downloading for each entry, the web url's are scraped from the provided entries and only those urls are downloaded. The icon is set for all entries that share a URL. This is useful if a group contains many entries that point to the same url, only 1 download call will occur. * The icon download dialog displays whether you are doing one entry, many entries, or an entire group. It is also modal so you have to dismiss it to use KeePassXC again. * The timeout setting is always enabled. * Moved DuckDuckGo fallback notice into the download dialog.

vraravam · 2019-07-08T03:46:24Z

Can a new beta be released with this? I would love to test this out.

droidmonkey · 2019-07-08T10:36:46Z

Yes absolutely, we are having trouble with our CI artifacts atm

droidmonkey · 2019-07-08T13:39:29Z

@vraravam I got the Mac and Windows artifacts up on https://snapshot.keepassxc.org

vraravam · 2019-07-08T14:40:48Z

tx @droidmonkey - downloading now.

vraravam · 2019-07-08T15:01:49Z

unfortunately, open locked db with touchId is not working in Mac when I use this version.

vraravam · 2019-07-08T15:05:45Z

Luckily, reverting to the 2.4.3 version is still able to read/decrypt the newly saved db (i had only used this feature to re-download all icons and not made any other changes).

droidmonkey · 2019-07-08T16:34:31Z

TouchID never works with snapshots. It is only available with SIGNED releases. You can self-sign the snapshot if you want to get this functionality.

vraravam · 2019-07-08T16:38:12Z

oh ok - that explains it. But, I was able to test the "download icons" for each node in the tree (except that it was disabled for the root node) in the db.
I dont think the nested trees worked though - in case that was supposed to.

droidmonkey · 2019-07-08T17:57:03Z

It does it at the group level and not for child groups. That is by design to limit the scope.

Added - Add 'Paper Backup' aka 'Export to HTML file' to the 'Database' menu [[#3277](#3277)] - Add statistics panel with information about the database (number of entries, number of unique passwords, etc.) to the Database Settings dialog [[#2034](#2034)] - Add offline user manual accessible via the 'Help' menu [[#3274](#3274)] - Add support for importing 1Password OpVault files [[#2292](#2292)] - Implement Freedesktop.org secret storage DBus protocol so that KeePassXC can be used as a vault service by libsecret [[#2726](#2726)] - Add support for OnlyKey as an alternative to YubiKeys (requires yubikey-personalization >= 1.20.0) [[#3352](#3352)] - Add group sorting feature [[#3282](#3282)] - Add feature to download favicons for all entries at once [[#3169](#3169)] - Add word case option to passphrase generator [[#3172](#3172)] - Add support for RFC6238-compliant TOTP hashes [[#2972](#2972)] - Add UNIX man page for main program [[#3665](#3665)] - Add 'Monospaced font' option to the notes field [[#3321](#3321)] - Add support for key files in auto open [[#3504](#3504)] - Add search field for filtering entries in Auto-Type dialog [[#2955](#2955)] - Complete usernames based on known usernames from other entries [[#3300](#3300)] - Parse hyperlinks in the notes field of the entry preview pane [[#3596](#3596)] - Allow abbreviation of field names in entry search [[#3440](#3440)] - Allow setting group icons recursively [[#3273](#3273)] - Add copy context menu for username and password in Auto-Type dialog [[#3038](#3038)] - Drop to background after copying a password to the clipboard [[#3253](#3253)] - Add 'Lock databases' entry to tray icon menu [[#2896](#2896)] - Add option to minimize window after unlocking [[#3439](#3439)] - Add option to minimize window after opening a URL [[#3302](#3302)] - Request accessibility permissions for Auto-Type on macOS [[#3624](#3624)] - Browser: Add initial support for multiple URLs [[#3558](#3558)] - Browser: Add entry-specific browser integration settings [[#3444](#3444)] - CLI: Add offline HIBP checker (requires a downloaded HIBP dump) [[#2707](#2707)] - CLI: Add 'flatten' option to the 'ls' command [[#3276](#3276)] - CLI: Add password generation options to `Add` and `Edit` commands [[#3275](#3275)] - CLI: Add XML import [[#3572](#3572)] - CLI: Add CSV export to the 'export' command [[#3278](#3278)] - CLI: Add `-y --yubikey` option for YubiKey [[#3416](#3416)] - CLI: Add `--dry-run` option for merging databases [[#3254](#3254)] - CLI: Add group commands (mv, mkdir and rmdir) [[#3313](#3313)]. - CLI: Add interactive shell mode command `open` [[#3224](#3224)] Changed - Redesign database unlock dialog [ [#3287](#3287)] - Rework the entry preview panel [ [#3306](#3306)] - Move notes to General tab on Group Preview Panel [[#3336](#3336)] - Enable entry actions when editing an entry and cleanup entry context menu [[#3641](#3641)] - Improve detection of external database changes [[#2389](#2389)] - Warn if user is trying to use a KDBX file as a key file [[#3625](#3625)] - Add option to disable KeePassHTTP settings migrations prompt [[#3349](#3349), [#3344](#3344)] - Re-enabled Wayland support (no Auto-Type yet) [[#3520](#3520), [#3341](#3341)] - Add icon to 'Toggle Window' action in tray icon menu [[3244](#3244)] - Merge custom data between databases only when necessary [[#3475](#3475)] - Improve various file-handling related issues when picking files using the system's file dialog [[#3473](#3473)] - Add 'New Entry' context menu when no entries are selected [[#3671](#3671)] - Reduce default Argon2 settings from 128 MiB and one thread per CPU core to 64 MiB and two threads to account for lower-spec mobile hardware [ [#3672](#3672)] - Browser: Remove unused 'Remember' checkbox for HTTP Basic Auth [[#3371](#3371)] - Browser: Show database name when pairing with a new browser [[#3638](#3638)] - Browser: Show URL in allow access dialog [[#3639](#3639)] - CLI: The password length option `-l` for the CLI commands `Add` and `Edit` is now `-L` [[#3275](#3275)] - CLI: The `-u` shorthand for the `--upper` password generation option has been renamed to `-U` [[#3275](#3275)] - CLI: Rename command `extract` to `export`. [[#3277](#3277)] Fixed - Improve accessibility for assistive technologies [[#3409](#3409)] - Correctly unlock all databases if `--pw-stdin` is provided [[#2916](#2916)] - Fix password generator issues with special characters [[#3303](#3303)] - Fix KeePassXC interrupting shutdown procedure [[#3666](#3666)] - Fix password visibility toggle button state on unlock dialog [[#3312](#3312)] - Fix potential data loss if database is reloaded while user is editing an entry [[#3656](#3656)] - Fix hard-coded background color in search help popup [[#3001](#3001)] - Fix font choice for password preview [[#3425](#3425)] - Fix handling of read-only files when autosave is enabled [[#3408](#3408)] - Handle symlinks correctly when atomic saves are disabled [[#3463](#3463)] - Enable HighDPI icon scaling on Linux [[#3332](#3332)] - Make Auto-Type on macOS more robust and remove old Carbon API calls [[#3634](#3634), [[#3347](#3347))] - Hide Share tab if KeePassXC is compiled without KeeShare support and other minor KeeShare improvements [[#3654](#3654), [[#3291](#3291), [#3029](#3029), [#3031](#3031), [#3236](#3236)] - Correctly bring window to the front when clicking tray icon on macOS [[#3576](#3576)] - Correct application shortcut created by MSI Installer on Windows [[#3296](#3296)] - Fix crash when removing custom data [[#3508](#3508)] - Fix placeholder resolution in URLs [[#3281](#3281)] - Fix various inconsistencies and platform-dependent compilation bugs [[#3664](#3664), [#3662](#3662), [#3660](#3660), [#3655](#3655), [#3649](#3649), [#3417](#3417), [#3357](#3357), [#3319](#3319), [#3318](#3318), [#3304](#3304)] - Browser: Fix potential leaking of entries through the browser integration API if multiple databases are opened [[#3480](#3480)] - Browser: Fix password entropy calculation [[#3107](#3107)] - Browser: Fix Windows registry settings for portable installation [[#3603](#3603)]

erm3nda · 2019-12-20T23:34:24Z

About the scope, i do remember older versions that had the hability to expand them, and show items of the child groups. I always use the Plugin or the Search feature, but I missed that feature on the new one.

I missed the "Download all Icons" first time i tried because it was greyed, so i never tried to apply on subgroups. My fail :-(

I don't think that the curren behavior helps anyone having +50 categories.
Seems a must to be able to propagate the thing around all subgroups.

Thank you.

varjolintu added new feature bounty 💰 labels May 21, 2019

varjolintu added this to the v2.5.0 milestone May 21, 2019

varjolintu mentioned this pull request May 21, 2019

Download favicon for all entries without icon [$40 awarded] #735

Closed

varjolintu force-pushed the feature/download_all_favicons branch 2 times, most recently from 3283d54 to 3935a8f Compare May 21, 2019 07:23

droidmonkey requested review from droidmonkey, phoerious and weslly and removed request for droidmonkey and phoerious May 21, 2019 15:27

weslly suggested changes May 22, 2019

View reviewed changes

weslly approved these changes May 23, 2019

View reviewed changes

varjolintu force-pushed the feature/download_all_favicons branch from 4c77859 to 4222a37 Compare May 24, 2019 06:19

varjolintu changed the title ~~WIP: Download all favicons~~ Download all favicons May 24, 2019

varjolintu removed the PR: In Progress label May 24, 2019

droidmonkey requested changes May 24, 2019

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

src/gui/DatabaseWidget.cpp Outdated Show resolved Hide resolved

src/gui/DatabaseWidget.cpp Outdated Show resolved Hide resolved

src/gui/DatabaseWidget.cpp Outdated Show resolved Hide resolved

varjolintu force-pushed the feature/download_all_favicons branch from 4222a37 to a90a35f Compare May 24, 2019 15:32

droidmonkey closed this May 31, 2019

droidmonkey requested changes Jun 14, 2019

View reviewed changes

src/gui/DatabaseWidget.cpp Outdated Show resolved Hide resolved

varjolintu force-pushed the feature/download_all_favicons branch from f4ea5a4 to 492e258 Compare June 15, 2019 06:07

droidmonkey force-pushed the feature/download_all_favicons branch from 492e258 to 8243e79 Compare June 27, 2019 21:35

droidmonkey approved these changes Jun 27, 2019

View reviewed changes

droidmonkey force-pushed the feature/download_all_favicons branch from 8243e79 to bf25a0b Compare June 27, 2019 23:54

varjolintu and others added 2 commits July 7, 2019 14:25

Download all favicons

c6b796e

droidmonkey force-pushed the feature/download_all_favicons branch from bf25a0b to b2a4c75 Compare July 7, 2019 18:39

droidmonkey merged commit 6ae27fa into keepassxreboot:develop Jul 7, 2019

varjolintu deleted the feature/download_all_favicons branch July 8, 2019 06:50

stdedos mentioned this pull request Jan 28, 2020

Add option to prefer DuckDuckGo #2996

Merged

phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download all favicons #3169

Download all favicons #3169

varjolintu commented May 21, 2019 •

edited

Loading

rugk commented May 21, 2019

varjolintu commented May 21, 2019

rugk commented May 21, 2019

varjolintu commented May 21, 2019

tomhundt commented May 21, 2019

droidmonkey commented May 21, 2019

varjolintu commented May 21, 2019 •

edited

Loading

varjolintu commented May 24, 2019

droidmonkey left a comment

varjolintu commented May 24, 2019

DJCrashdummy commented Jun 14, 2019

varjolintu commented Jun 14, 2019

DJCrashdummy commented Jun 14, 2019

droidmonkey left a comment

droidmonkey commented Jun 27, 2019

varjolintu commented Jun 28, 2019

droidmonkey commented Jun 28, 2019

vraravam commented Jul 8, 2019

droidmonkey commented Jul 8, 2019

droidmonkey commented Jul 8, 2019

vraravam commented Jul 8, 2019

vraravam commented Jul 8, 2019

vraravam commented Jul 8, 2019

droidmonkey commented Jul 8, 2019 •

edited

Loading

vraravam commented Jul 8, 2019 •

edited

Loading

droidmonkey commented Jul 8, 2019

erm3nda commented Dec 20, 2019

Download all favicons #3169

Download all favicons #3169

Conversation

varjolintu commented May 21, 2019 • edited Loading

Type of change

Description and Context

Screenshots

Testing strategy

Checklist:

rugk commented May 21, 2019

varjolintu commented May 21, 2019

rugk commented May 21, 2019

varjolintu commented May 21, 2019

tomhundt commented May 21, 2019

droidmonkey commented May 21, 2019

varjolintu commented May 21, 2019 • edited Loading

varjolintu commented May 24, 2019

droidmonkey left a comment

Choose a reason for hiding this comment

varjolintu commented May 24, 2019

DJCrashdummy commented Jun 14, 2019

varjolintu commented Jun 14, 2019

DJCrashdummy commented Jun 14, 2019

droidmonkey left a comment

Choose a reason for hiding this comment

droidmonkey commented Jun 27, 2019

varjolintu commented Jun 28, 2019

droidmonkey commented Jun 28, 2019

vraravam commented Jul 8, 2019

droidmonkey commented Jul 8, 2019

droidmonkey commented Jul 8, 2019

vraravam commented Jul 8, 2019

vraravam commented Jul 8, 2019

vraravam commented Jul 8, 2019

droidmonkey commented Jul 8, 2019 • edited Loading

vraravam commented Jul 8, 2019 • edited Loading

droidmonkey commented Jul 8, 2019

erm3nda commented Dec 20, 2019

varjolintu commented May 21, 2019 •

edited

Loading

varjolintu commented May 21, 2019 •

edited

Loading

droidmonkey commented Jul 8, 2019 •

edited

Loading

vraravam commented Jul 8, 2019 •

edited

Loading