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 option to prefer DuckDuckGo #2996

Merged
merged 2 commits into from
Apr 17, 2019
Merged

Add option to prefer DuckDuckGo #2996

merged 2 commits into from
Apr 17, 2019

Conversation

xf-
Copy link
Contributor

@xf- xf- commented Apr 13, 2019

Type of change

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

Description and Context

Some systems provide a favicon.ico out-of-box and set the correct icon via source code. Keepassxc will load the out-of-box icon and not a recognizable/brand icon. Only fix is by hand, because DuckDuckGo is only a fallback.
This patch adds an option to prefer DuckDuckGo over direct Favicon download.

(I tried to open an issue, but it got declined. Now i wrote a patch and fixed all icons in my kdbx.)

Fixes #2557

Screenshots

image

Testing strategy

Build works, test run, format only changes not related lines.

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]
  • ✅ My change requires a change to the documentation, and I have updated it accordingly.

@droidmonkey
Copy link
Member

I prefer no setting and just use duckduckgo full stop if that setting is enabled.

@xf-
Copy link
Contributor Author

xf- commented Apr 13, 2019

@droidmonkey also a nice way. DuckDuckGo supports multiple definitions of favicons and should return the correct one. Only small disadvantage is the possible cache (Quote: Updates to favicons should occur automatically over a period of a few weeks.). As fallback (e.g. local network) is direct download.

Other idea was a second button to force the second way, but that would complicate the UI or load both and select in popover (like advanced page into) has the same problem.

QUrl fallbackUrl = QUrl("https://icons.duckduckgo.com");
fallbackUrl.setPath("/ip3/" + QUrl::toPercentEncoding(fullyQualifiedDomain) + ".ico");

m_urlsToTry.append(fallbackUrl);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the new setting and just change this from append to prepend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added ut afterwards to ensure the same order and hostIsIp afterwards

@xf-
Copy link
Contributor Author

xf- commented Apr 14, 2019

@droidmonkey Any rewording, because it is not a fallback option anymore

@droidmonkey
Copy link
Member

"Use DuckDuckGo to download website icons"

@xf-
Copy link
Contributor Author

xf- commented Apr 14, 2019

Changed it - ts files are loaded via translation service. didn't touch the source string.

src/gui/EditWidgetIcons.cpp Outdated Show resolved Hide resolved
@droidmonkey droidmonkey added this to the v2.4.2 milestone Apr 15, 2019
@droidmonkey droidmonkey changed the base branch from develop to release/2.4.2 April 15, 2019 18:25
Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the necessary changes

@droidmonkey droidmonkey merged commit 42d34a1 into keepassxreboot:release/2.4.2 Apr 17, 2019
droidmonkey added a commit that referenced this pull request May 31, 2019
- Improve resilience against memory attacks - overwrite memory before free [#3020]
- Prevent infinite save loop when location is unavailable [#3026]
- Attempt to fix quitting application when shutdown or logout issued [#3199]
- Support merging database custom data [#3002]
- Fix opening URL's with non-http schemes [#3153]
- Fix data loss due to not reading all database attachments if duplicates exist [#3180]
- Fix entry context menu disabling when using keyboard navigation [#3199]
- Fix behaviors when canceling an entry edit [#3199]
- Fix processing of tray icon click and doubleclick [#3112]
- Update group in preview widget when focused [#3199]
- Prefer DuckDuckGo service over direct icon download (increases resolution) [#2996]
- Remove apply button in application settings [#3019]
- Use winqtdeploy on Windows to correct deployment issues [#3025]
- Don't mark entry edit as modified when attribute selection changes [#3041]
- Use console code page CP_UTF8 on Windows if supported [#3050]
- Snap: Fix locking database with session lock [#3046]
- Snap: Fix theming across Linux distributions [#3057]
- Snap: Use SNAP_USER_COMMON and SNAP_USER_DATA directories [#3131]
- KeeShare: Automatically enable WITH_XC_KEESHARE_SECURE if quazip is found [#3088]
- macOS: Fix toolbar text when in dark mode [#2998]
- macOS: Lock database on switching user [#3097]
- macOS: Fix global Auto-Type when the database is locked [#3138]
- Browser: Close popups when database is locked [#3093]
- Browser: Add tests [#3016]
- Browser: Don't create default group if custom group is enabled [#3127]
@stdedos
Copy link

stdedos commented Jan 28, 2020

Shouldn't there be a "fallback to download icons from a website's favicon"? Or add a secondary flow somehow (related to #3169, since that's my current interaction).
There are quite a few icons missing, and then there are local IPs too.

@droidmonkey
Copy link
Member

This PR puts duckduckgo first, then it tries the websites own ico.

@stdedos
Copy link

stdedos commented Jan 28, 2020

I tried to download a http://192.168.1.1/ entry.

HTML source for the favicon:
<link rel="shortcut icon" type="image/x-icon" href="../img/favicon.ico">

image

Should I do a bug report? 😕

@droidmonkey
Copy link
Member

droidmonkey commented Jan 28, 2020

We don't parse the HTML, we only look at ./favicon.ico

We will never parse the HTML

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants