Skip to content

Add a URL that preserves the URL path when trying to resolve favicons.#8565

Merged
droidmonkey merged 3 commits intokeepassxreboot:developfrom
libklein:hotfix/8564-fix-favicon-download
Oct 20, 2022
Merged

Add a URL that preserves the URL path when trying to resolve favicons.#8565
droidmonkey merged 3 commits intokeepassxreboot:developfrom
libklein:hotfix/8564-fix-favicon-download

Conversation

@libklein
Copy link
Copy Markdown
Contributor

@libklein libklein commented Oct 10, 2022

Add an additional URL that appends /favicon.ico to the path of the URL. This works as follows:
https://google.com/my/path will become https://google.com/my/favicon.ico and https://google.com/my/path/ will become https://google.com/my/path/favicon.ico.

Fixes #8564.

Testing strategy

Added unit tests.

@libklein
Copy link
Copy Markdown
Contributor Author

Im currently working on a PR that will fix various favicon download related issues by searching the page's HTML. This is just a quick fix that fixes this specific issue with the hope of making the backport.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 10, 2022

Codecov Report

Base: 64.44% // Head: 64.37% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (378d928) compared to base (56307e6).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 378d928 differs from pull request most recent head 08cffcf. Consider uploading reports for the commit 08cffcf to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8565      +/-   ##
===========================================
- Coverage    64.44%   64.37%   -0.07%     
===========================================
  Files          340      339       -1     
  Lines        44142    43996     -146     
===========================================
- Hits         28446    28319     -127     
+ Misses       15696    15677      -19     
Impacted Files Coverage Δ
src/gui/IconDownloader.cpp 45.83% <100.00%> (+2.01%) ⬆️
...rc/fdosecrets/widgets/SettingsWidgetFdoSecrets.cpp 56.06% <0.00%> (-3.03%) ⬇️
src/fdosecrets/dbus/DBusMgr.cpp 52.20% <0.00%> (-1.47%) ⬇️
src/gui/DatabaseOpenWidget.cpp 56.65% <0.00%> (-0.62%) ⬇️
src/gui/Application.cpp 31.56% <0.00%> (-0.41%) ⬇️
src/cli/Import.cpp 87.18% <0.00%> (-0.32%) ⬇️
src/gui/DatabaseTabWidget.cpp 61.05% <0.00%> (-0.16%) ⬇️
src/cli/Command.cpp 87.78% <0.00%> (-0.13%) ⬇️
src/gui/DatabaseWidget.cpp 61.16% <0.00%> (-0.04%) ⬇️
src/core/Config.h 100.00% <0.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@michaelk83
Copy link
Copy Markdown

Merge branch 'develop' into hotfix/8564-fix-favicon-download

@libklein FYI, the preferred approach based on their wiki to rebase, rather than merge:
https://github.com/keepassxreboot/keepassxc/wiki/Merge-strategy#merging-strategy-and-requirements

Copy link
Copy Markdown
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Looks good to me. Might even be worth a backport, what do you think @droidmonkey?

@phoerious phoerious added this to the v2.7.2 milestone Oct 18, 2022
@droidmonkey droidmonkey added bug pr: backported Pull request backported to previous release labels Oct 20, 2022
@droidmonkey droidmonkey merged commit 1d00c22 into keepassxreboot:develop Oct 20, 2022
pull bot pushed a commit to Tiamat-Tech/keepassxc that referenced this pull request Oct 20, 2022
@phoerious phoerious added pr: bugfix Pull request fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: backported Pull request backported to previous release pr: bugfix Pull request fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

favicon "download" only pulls from root website

5 participants