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

Support sorting HTML export #7011

Conversation

libklein
Copy link
Contributor

@libklein libklein commented Oct 3, 2021

Resolved #6164.

  • Implement sorting support in HtmlExporter
  • Add ExportDialog class and UI, which allows to configure export options.

Screenshots

Current integration:
kpx1

The export dialog:
kpx2

Export in descending order:
descending-order
Export in ascending order:
ascending-order
Export in database order:
database-order

Testing strategy

Test on local databases.

Type of change

Open issues

  • The "ascending" option has no effect when database order is selected. Should the checkbox be disabled in that case or should we remove the "database order" option? I think it probably only causes confusion anyway.
  • How about testing? I'd propose to compare the generated .html files to expected output on a test database. I'd reuse the ones in tests/data/*.kdbx for this purpose.

Discussion / Future work

Common Exporter Interfance

Both CSV & HTML Exporter share a lot of functionality, especially if we want to include support for sorting, exporting only specific groups, etc. in the future. This should also make implementing new exporters easier. I think we should address this issue and refactor in a future PR.

  • Common Exporter Dialog
  • Move Enum to Exporter

@codecov-commenter
Copy link

codecov-commenter commented Oct 3, 2021

Codecov Report

Merging #7011 (231662c) into develop (d3b28f8) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #7011      +/-   ##
===========================================
- Coverage    64.21%   64.12%   -0.10%     
===========================================
  Files          334      335       +1     
  Lines        42221    42284      +63     
===========================================
  Hits         27111    27111              
- Misses       15110    15173      +63     
Impacted Files Coverage Δ
src/gui/DatabaseTabWidget.cpp 65.33% <0.00%> (+1.28%) ⬆️
src/gui/HtmlExporter.cpp 0.00% <0.00%> (ø)
src/gui/export/ExportDialog.cpp 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3b28f8...231662c. Read the comment docs.

@libklein
Copy link
Contributor Author

libklein commented Oct 3, 2021

Could someone perhaps give some hints/point me to some information regarding the translation failure?

Also, are there any options on the future work / open issues sections?

@droidmonkey
Copy link
Member

droidmonkey commented Oct 4, 2021

For translation run ./release-tool i18n lupdate

I'm not sure what you mean by your second question

@libklein libklein marked this pull request as ready for review October 7, 2021 20:25
@libklein
Copy link
Contributor Author

libklein commented Oct 8, 2021

Thanks! Did that.

I was talking about the questions mentioned in the "open issues" section of the PR:

The "ascending" option has no effect when database order is selected. Should the checkbox be disabled in that case or should we remove the "database order" option? I think it probably only causes confusion anyway.

and

How about testing? I'd propose to compare the generated .html files to expected output on a test database. I'd reuse the ones in tests/data/*.kdbx for this purpose.

@droidmonkey
Copy link
Member

droidmonkey commented Oct 8, 2021

I would remove the Ascending selection box entirely. Give three choices in the "Sort by..." drop down:

  • Name Ascending (A-Z)
  • Name Descending (Z-A)
  • Database Order

For testing, I am not too hung up on test cases for this, it isn't a critical feature. If you want to write them though, use the NewDatabase.kdbx

@libklein
Copy link
Contributor Author

libklein commented Oct 9, 2021

The original idea behind the "Ascending" checkbox was that i'd like to extend the system to cover several exporters/be more generic. If output is sortable by several columns, then an ascending checkbox is quickly worth it's weight.

I think this is out of scope for this PR though. I've hence updated the dialog according to your suggestions.

I have a final question though: How do you properly associate an enum value with a string? Is there some way to leverage the Qt Property/Meta Type system? I've used a QMap for this purpose (with fallback to the enum key). I've tried to wrap the enums key in a call to tr() and insert the proper strings in the translation table, but that does not work with automatic translation generation :/.

@droidmonkey
Copy link
Member

Use a function that maps an enum to a string in a switch statement.

For extending to this other exporters, like csv, it would be far more useful to have a "sort column" choice in addition to the three sort by choices. Ascending checkbox doesn't make sense on its own.

@libklein
Copy link
Contributor Author

Thanks @droidmonkey ! I've updated the code accordingly. It should be ready for merging now.

@droidmonkey droidmonkey self-requested a review October 10, 2021 15:47
@droidmonkey droidmonkey added this to the v2.7.0 milestone Oct 10, 2021
@droidmonkey droidmonkey changed the title Implement feature 6164: Alphabetized HTML Export. Support sorting HTML export Oct 11, 2021
@libklein
Copy link
Contributor Author

Is there anything else that needs to be done? If not, could you perhaps label the PR as "hacktoberfest-accepted" (see https://hacktoberfest.digitalocean.com/resources/maintainers)? I

@droidmonkey
Copy link
Member

I might do some style changes but otherwise I'll approve the PR for Hacktoberfest reasons

@droidmonkey droidmonkey force-pushed the feature/6164-alphabetized-html-export branch from a6edc5f to 92de8b0 Compare November 23, 2021 14:34
- Closes keepassxreboot#6164
- Implement sorting support in HtmlExporter
- Add ExportDialog class and UI, which allows to configure export options.
@droidmonkey droidmonkey force-pushed the feature/6164-alphabetized-html-export branch from 92de8b0 to 231662c Compare November 24, 2021 04:53
@droidmonkey
Copy link
Member

I made the changes I wanted to make (simplified the code quite a bit) and this is ready for merge.

@droidmonkey droidmonkey merged commit 296cbf0 into keepassxreboot:develop Nov 24, 2021
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.

Ability to have export to HTML File Alphabetized
3 participants