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.

Sign up for GitHub

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

Jump to bottom

Fix Best-Matching ..again #5316

Merged
merged 2 commits into from
Sep 13, 2020
Merged

Fix Best-Matching ..again #5316

merged 2 commits into from
Sep 13, 2020

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Aug 22, 2020 •
edited
Loading

It was still broken. And actually the change for passing a full URL from the extension fixed the issue without any extra handling in the BrowserService.cpp.

Fixes #5299.

Testing strategy

Added some more tests, and tested it also manually. Double verification will be needed. This should be the final fix for this. Really.

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

@varjolintu varjolintu added the bug label Aug 22, 2020
@varjolintu varjolintu added this to the v2.6.2 milestone Aug 22, 2020
@varjolintu varjolintu force-pushed the fix/best-matching-again branch from 41fd664 to f2156da Compare August 22, 2020 11:48
@varjolintu varjolintu mentioned this pull request Aug 22, 2020
Closed
@droidmonkey
Copy link
Member

The whole logic tree needs a refactor. I suspect we are also not handling multiple urls correctly.

@varjolintu
Copy link
Member Author

@droidmonkey That needs to be checked too.

@varjolintu
Copy link
Member Author

varjolintu commented Aug 23, 2020 •
edited
Loading

Tested the Additional URL's behavior with the following scenario:

  • Three entries with URL's https://github.com/loginpage, https://test.github.com/ and https://github.com/
  • Added Additional URL https://test.github.com/anotherpage to the first entry
  • Requested URL https://test.github.com/anotherpage
  • Sorting only returns the second entry as a priority. It should return the first one instead because the URL matches better.

EDIT: And of course, I'm making a test case.

EDIT 2: Made a change inside BrowserService::sortPriority() that also retrieves the sorting number for Additional URL's in the entry, stores all in a list (including entry's own URL) and returns the max value. It works quite well.

@droidmonkey
Copy link
Member

Is this ready for review?

@varjolintu
Copy link
Member Author

@droidmonkey Yes.

droidmonkey pushed a commit to varjolintu/keepassxc that referenced this pull request Aug 31, 2020
@droidmonkey
Fix Best-Matching browser extension ..again
22e59f3 
* Fixes keepassxreboot#5316
* Added some more tests
@droidmonkey droidmonkey force-pushed the fix/best-matching-again branch from b942172 to 22e59f3 Compare August 31, 2020 17:21
@droidmonkey droidmonkey changed the base branch from develop to release/2.6.2 August 31, 2020 17:21
droidmonkey
droidmonkey requested changes Aug 31, 2020
View reviewed changes
src/browser/BrowserService.cpp Show resolved Hide resolved
src/browser/BrowserService.cpp Outdated Show resolved Hide resolved
@droidmonkey
Fix Browser URL Best-Matching ..again
97d70b0 
* Handle Additional URLs when doing priority sorting
* Clean up function calls
* Fixes keepassxreboot#5299
@droidmonkey droidmonkey force-pushed the fix/best-matching-again branch 3 times, most recently from 3a38703 to 421f499 Compare September 6, 2020 22:00
@droidmonkey droidmonkey force-pushed the fix/best-matching-again branch from 421f499 to 577bbce Compare September 12, 2020 20:39
droidmonkey
droidmonkey approved these changes Sep 12, 2020
View reviewed changes
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.

Fixed remaining issues with Qt < 5.11

@droidmonkey droidmonkey merged commit e391dd1 into keepassxreboot:release/2.6.2 Sep 13, 2020
@varjolintu varjolintu deleted the fix/best-matching-again branch September 13, 2020 17:08
@simonschaufi simonschaufi mentioned this pull request Sep 29, 2020
Closed
phoerious added a commit that referenced this pull request Oct 21, 2020
@phoerious
Release 2.6.2
e9b9582 
Added

- Add option to keep window always on top to view menu [#5542]
- Move show/hide usernames and passwords to view menu [#5542]
- Add command line options and environment variables for changing the config locations [#5452]
- Include TOTP settings in CSV import/export and add support for ISO datetimes [#5346]

Changed

- Mask sensitive information in command execution confirmation prompt [#5542]
- SSH Agent: Avoid shortcut conflict on macOS by changing "Add key" to Ctrl+H on all platforms [#5484]

Fixed

- Prevent data loss with drag and drop between databases [#5536]
- Fix crash when toggling Capslock rapidly [#5545]
- Don't mark URL references as invalid URL [#5380]
- Reset entry preview after search [#5483]
- Set Qt::Dialog flag on database open dialog [#5356]
- Fix sorting of database report columns [#5426]
- Fix IfDevice matching logic [#5344]
- Fix layout issues and a stray scrollbar appearing on top of the entry edit screen [#5424]
- Fix tabbing into the notes field [#5424]
- Fix password generator ignoring settings on load [#5340]
- Restore natural entry sort order on application load [#5438]
- Fix paperclip and TOTP columns not saving state [#5327]
- Enforce fixed password font in entry preview [#5454]
- Add scrollbar when new database wizard exceeds screen size [#5560]
- Do not mark database as modified when viewing Auto-Type associations [#5542]
- CLI: Fix two heap-use-after-free crashes [#5368,#5470]
- Browser: Fix key exchange not working with multiple simultaneous users on Windows [#5485]
- Browser: Fix entry retrieval when "only best matching" is enabled [#5316]
- Browser: Ignore recycle bin on KeePassHTTP migration [#5481]
- KeeShare: Fix import crash [#5542]
- macOS: Fix toolbar theming and breadcrumb display issues [#5482]
- macOS: Fix file dialog randomly closing [#5479]
- macOS: Fix being unable to select OPVault files for import [#5341]
This was referenced Dec 2, 2020
Closed
Closed
@phoerious phoerious added pr: bugfix Pull request that 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
Reviewers

@MaxXor MaxXor MaxXor approved these changes

@droidmonkey droidmonkey droidmonkey approved these changes

@phoerious phoerious Awaiting requested review from phoerious

Assignees
No one assigned
Labels
feature: Browser pr: bugfix Pull request that fixes a bug
Projects
None yet
Milestone
v2.6.2
Development

Successfully merging this pull request may close these issues.

2.6.1: browser does not find entry
4 participants
@varjolintu @droidmonkey @MaxXor @phoerious