Refactor compare engines to use DiffContext#3197
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors the compare engine classes (BinaryCompare, TimeSizeCompare, ExistenceCompare, and ImageCompare) to receive configuration through a CDiffContext reference passed to their constructors, rather than through individual method parameters. This simplifies the API by consolidating context information (comparison method, number of files, options) into a single context object.
Changes:
- Modified all compare engine constructors to accept a CDiffContext reference and store it as a member variable
- Updated CompareFiles methods to use context information from the stored CDiffContext instead of method parameters
- Updated production code (FolderCmp.cpp, MergeFrameCommon.cpp) to pass CDiffContext when constructing compare engines
- Updated test files to create CDiffContext objects with appropriate PathContext for 2-way and 3-way comparisons
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Src/CompareEngines/BinaryCompare.h | Changed constructor to accept CDiffContext reference; removed SetAbortable method; CompareFiles no longer takes PathContext parameter |
| Src/CompareEngines/BinaryCompare.cpp | Constructor simplified to inline initialization; CompareFiles now calls GetComparePaths and GetAbortable from stored context; changed IAbortable parameter to const |
| Src/CompareEngines/TimeSizeCompare.h | Added member variables for nfiles, compMethod, ignoreSmallDiff; removed SetAdditionalOptions method; CompareFiles signature simplified |
| Src/CompareEngines/TimeSizeCompare.cpp | Constructor initializes members from CDiffContext; removed SetAdditionalOptions implementation; CompareFiles uses member variables instead of parameters; changed auto to constexpr auto |
| Src/CompareEngines/ExistenceCompare.h | Added m_nfiles member; changed constructor and CompareFiles signatures |
| Src/CompareEngines/ExistenceCompare.cpp | Constructor initializes m_nfiles from context; CompareFiles uses m_nfiles instead of parameter |
| Src/CompareEngines/ImageCompare.h | Changed constructor to accept CDiffContext; removed SetColorDistanceThreshold method; CompareFiles signature simplified; added m_ctxt member |
| Src/CompareEngines/ImageCompare.cpp | Constructor initializes color distance threshold from context; CompareFiles calls GetComparePaths from stored context |
| Src/FolderCmp.cpp | Updated to pass *m_pCtxt when constructing compare engines; removed calls to SetAdditionalOptions, SetAbortable, SetColorDistanceThreshold; removed PathContext creation before CompareFiles calls |
| Src/MergeFrameCommon.cpp | Creates CDiffContext and passes it to BinaryCompare constructor; updated file path setup for DIFFITEM |
| Testing/GoogleTest/BinaryCompare/BinaryCompare_test.cpp | Tests now create CDiffContext objects for 2-way and 3-way comparisons; updated to set file info via SetFile method |
| Testing/GoogleTest/ExistenceCompare/ExistenceCompare_test.cpp | Tests now create CDiffContext objects for 2-way and 3-way comparisons |
| Testing/GoogleTest/TimeSizeCompare/TimeSizeCompare_test.cpp | Tests now create CDiffContext objects for 2-way and 3-way comparisons; removed SetAdditionalOptions calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
Src/MergeFrameCommon.cpp:86
- The error message is misleading when the comparison fails. It says "Selected files are identical (with current settings)" when in fact a comparison error occurred. The message should indicate that the comparison could not be completed. Additionally, this code does not handle the CMPABORT case (when the user cancels the operation) separately from errors.
if (di.diffcode.isResultError())
return _("Selected files are identical (with current settings).\r\nBut binary comparison failed.");
return di.diffcode.isResultSame()
? _("Selected files are identical (binary match).")
: _("Selected files are identical (with current settings).\r\nBut differ at the binary level.");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.