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

Fix sorting of database report columns #5426

Merged
merged 1 commit into from
Sep 15, 2020
Merged

Fix sorting of database report columns #5426

merged 1 commit into from
Sep 15, 2020

Conversation

jacopotediosi
Copy link

Fixed columns sorting in HealthCheck and HIBP widgets in database report (#4976).

I'm not a Qt developer, but I wanted to try to contribute anyway; so if this PR does not reflect the programming patterns of this project, please feel free to reject it.

Since in both widgets the columns need to be sorted in a particular way (eg in HIBP widget the exposed password numbers are displayed as words but they must be sorted as digits), in this PR I suggest to apply Qt::UserRole as SortRole to the tables. In this way, each field has two values: a textual one which is displayed to the user and one set with setData() for internal use for sorting.

I also added that by default the tables are sorted in order to have the most exposed passwords or passwords with the lowest score at the top.

Screenshots

immagine
immagine

Testing strategy

I have visually tested my changes before doing the PR and they seem to be ok (no compilation errors, no crashes found, all unit tests passed, sorting seems to work with both positive and negative numbers).

Type of change

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

@droidmonkey
Copy link
Member

droidmonkey commented Sep 13, 2020

Unfortunately this isn't a fix to the problem for every column.

@jacopotediosi
Copy link
Author

Unfortunately this isn't a fix to the problem for every column.

With this PR the columns use for sorting the values as follows:
Health Check:

  • Descr (color): use numerical score
  • Title: Alphabetical (title as string)
  • Path: Alphabetical (path as string)
  • Score: use numerical score
  • Reason: Alphabetical (reason as string)

HIBP:

  • Title: Alphabetical (title as string)
  • Path: Alphabetical (path as string)
  • Exposed: the numerical part of the sentence

What are the columns that don't convince you?
In my opinion the only difficult to sort column that remains is the Reason one, because the values that are entered in that field are of a "mixed" type and I don't find a sorting logic that makes sense.

@droidmonkey
Copy link
Member

I redid your code to use standard Qt methods and also allow proper sorting of the HIBP count column.

@droidmonkey droidmonkey added this to the v2.6.2 milestone Sep 14, 2020
@droidmonkey droidmonkey changed the base branch from develop to release/2.6.2 September 14, 2020 03:18
src/gui/reports/ReportsWidgetHibp.cpp Show resolved Hide resolved
src/gui/reports/ReportsWidgetHibp.cpp Show resolved Hide resolved
src/gui/reports/ReportsWidgetHealthcheck.cpp Show resolved Hide resolved
src/gui/reports/ReportsWidgetHealthcheck.cpp Outdated Show resolved Hide resolved
src/gui/reports/ReportsWidgetHibp.cpp Outdated Show resolved Hide resolved
@droidmonkey
Copy link
Member

Sorry I totally screwed this up by rebasing my commit over yours.

@droidmonkey
Copy link
Member

All fixed now.

@droidmonkey droidmonkey merged commit 3c5bd0f into keepassxreboot:release/2.6.2 Sep 15, 2020
phoerious added a commit that referenced this pull request Oct 21, 2020
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]
@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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants