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

Implement best matches only option with browser integration #1822

Merged

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Apr 7, 2018

Returning only the best matched entries option hasn't worked properly. This fixes it and only returns entries with sort priority 100.

Description

It's not perfect but a small improvement. If you can think anything that could improve this PR please let me know.

Motivation and context

Fixes
keepassxreboot/keepassxc-browser#84

Fixes #1112

How has this been tested?

Manually.

Types of changes

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

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]

@varjolintu varjolintu added this to the v2.3.2 milestone Apr 7, 2018
@varjolintu varjolintu changed the title Implement best matches only option Implement best matches only option with browser integration Apr 7, 2018
@rugk
Copy link

rugk commented Apr 8, 2018

Better add "fixes" or so, before each issue you link, so it is automatically closed when a PR is merged. (I know, third time I comment that. 😄)

@rugk
Copy link

rugk commented Apr 8, 2018

BTW as for me I could not even find that setting in the UI in KeePassXC. The description here sounds as if the option is there, but just did not work. Or did you hide it deliberately, because you needed to make it work?

@varjolintu
Copy link
Member Author

The setting has been hidden since the new browser integration was merged because it was not implemented.

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.

One comment but otherwise fine

const int priority = sortPriority(entry, host, submitUrl, baseSubmitUrl);

// Ignore matches that aren't exact enough
if (BrowserSettings::bestMatchOnly() && priority < 100) {
Copy link
Member

Choose a reason for hiding this comment

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

Does 100 represent percent matching? If not, this might need some explaining.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Interesting... This will be a target for 2.4 haha

@varjolintu varjolintu force-pushed the best_matches_only branch 3 times, most recently from 85e8e19 to 1ae145e Compare May 4, 2018 21:41
@varjolintu varjolintu force-pushed the best_matches_only branch from 5eb2388 to c5845b1 Compare May 5, 2018 06:34
@varjolintu
Copy link
Member Author

Rebased. I think this is quite ready for merging.

@droidmonkey
Copy link
Member

droidmonkey commented May 5, 2018

I fixed an error where zero priority matches were being filtered by the sort function. We should really collapse a lot of the filtering and sorting happening in the browser service in 2.4.

@droidmonkey droidmonkey merged commit 48295ef into keepassxreboot:release/2.3.2 May 5, 2018
@varjolintu varjolintu deleted the best_matches_only branch May 6, 2018 07:49
droidmonkey added a commit that referenced this pull request May 8, 2018
- Enable high entropy ASLR on Windows [#1747]
- Enhance favicon fetching [#1786]
- Fix crash on Windows due to autotype [#1691]
- Fix dark tray icon changing all icons [#1680]
- Fix --pw-stdin not using getPassword function [#1686]
- Fix placeholders being resolved in notes [#1907]
- Enable auto-type start delay to be configurable [#1908]
- Browser: Fix native messaging reply size [#1719]
- Browser: Increase maximum buffer size [#1720]
- Browser: Enhance usability and functionality [#1810, #1822, #1830, #1884, #1906]
- SSH Agent: Parse aes-256-cbc/ctr keys [#1682]
- SSH Agent: Enhance usability and functionality [#1677, #1679, #1681, #1787]
jtl999 pushed a commit to jtl999/keepassxc that referenced this pull request Jul 29, 2018
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