search

COVESA / dlt-viewer

Diagnostic Log and Trace viewing program
Other
425 stars 240 forks source link

Testable search #505

Closed vifactor closed 3 days ago

vifactor commented 1 month ago

The search dialog core functionality is moved into a separate class.

Added unit tests for the class. Without this change I do not believe it is possible to make any "reliable" improvements for the search. The unit tests are so far run only locally. CI adjustments can be done later if this PR is merged

The "payload range" functionality is not implemented: it does not work in the master-branch and requirements are unclear.

Since in this PR I use optional and variant I also switch on c++17 standard where these utils were introduced. If not acceptable, in principle code can be rewritten without them. See https://github.com/COVESA/dlt-viewer/discussions/501

I tested the refactored search functionality on several dlt file manually, so far I did not see issues.

vifactor commented 1 month ago

@alexmucde any thoughts on this? in principle the PR is ready. do you want me to rewrite it without using c++17? would you see it to be done differently? what about getting rid of the broken (and probably unused) payload range functionality?

alexmucde commented 4 weeks ago

Is c++1z the name for C++17?

vifactor commented 4 weeks ago

Is c++1z the name for C++17?

yes, this is the only way I was able to enforce it for "Build macOS (macos-13, Xcode_15.2)"-job. AFAIR, Win, Lin and Mac-14 were green if I use CONFIG += c++17 syntax. Here is the source I used: https://stackoverflow.com/a/51197053

If we the project maintained only cmake build system, things would be simpler and cleaner. But I guess there is a reason you have to maintain both.