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

impr: Added CSV, TSV and JSON as export options for Find results #1673

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

SparkyTD
Copy link
Contributor

@SparkyTD SparkyTD commented May 13, 2024

Problem description

The default result export functionality of the Find tool is limited to only exporting data in a nonstandard text format. This PR adds support for exporting the results in CSV, TSV or JSON format. The PR also removes the old format.

Implementation description

I added the classes ExportFormatter, ExportFormatterCsv, ExportFormatterTsv and ExportFormatterJson, with similar implementations to the pattern data exporters.

I also moved the ViewFind::Occurrence class into hex/helpers/types.hh, so the exporters can access it.

Screenshots

image

Additional things

Another small change I made is moving the "{} entries found" line on the same line as the Search and Reset buttons. I think it looks cleaner this way, but if anyone disagrees, I can revert it.

@SparkyTD SparkyTD changed the title impr: Added CSV and TSV as export options for Find results impr: Added CSV, TSV and JSON as export options for Find results May 13, 2024
@SparkyTD
Copy link
Contributor Author

The CSV/TSV exporters have been tested by finding 5 character longs strings in /dev/urandom, and all the escaping logic seems to work as it's supposed to.

As for JSON, the nlohmann library already takes care of escaping strings.

@SparkyTD
Copy link
Contributor Author

There is an issue that causes the format selector popup to stay visible behind the main window in some cases when the user clicks the parent window. The bug also exists in the original pattern export popup. I haven't been able to fix it yet.

image

@SparkyTD SparkyTD marked this pull request as ready for review May 14, 2024 19:08
@WerWolv
Copy link
Owner

WerWolv commented May 19, 2024

Thanks a lot for the work!
I'd suggest adding these exporter registration things to the Content Registry instead, that way other plugins can re-use the same infrastructure you already wrote to add their own formatters. The Occurrences struct as well because now it's being imported by basically every single file in ImHex which is probably not needed :D

The actual implementations of the formatters can stay in the builtin plugin like it is

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 98 lines in your changes are missing coverage. Please review.

Please upload report for BASE (master@dedd99f). Learn more about missing BASE report.

Files Patch % Lines
...content/export_formatters/export_formatter_csv.hpp 0.00% 30 Missing ⚠️
plugins/builtin/source/content/views/view_find.cpp 0.00% 29 Missing ⚠️
...ontent/export_formatters/export_formatter_json.hpp 0.00% 14 Missing ⚠️
plugins/builtin/source/content/data_formatters.cpp 0.00% 12 Missing ⚠️
lib/libimhex/source/api/content_registry.cpp 0.00% 5 Missing ⚠️
...content/export_formatters/export_formatter_tsv.hpp 0.00% 4 Missing ⚠️
lib/libimhex/include/hex/api/content_registry.hpp 0.00% 2 Missing ⚠️
...ude/content/export_formatters/export_formatter.hpp 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##             master   #1673   +/-   ##
========================================
  Coverage          ?   1.50%           
========================================
  Files             ?     279           
  Lines             ?   27088           
  Branches          ?   14556           
========================================
  Hits              ?     409           
  Misses            ?   26421           
  Partials          ?     258           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants