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

TableViewer: fix calls to hashCode of DataSetsRow #343

Merged
merged 1 commit into from
Feb 3, 2021

Conversation

dedeibel
Copy link
Contributor

While working on a bugfix for the TableViewer I noticed a problem when the DataSetsRow was added to a Hashing collection. (For example the result of a fxRobot lookup, resulting in a Set).

The implementation of the DataSetsRow equals/hashCode seem to use to the model`s equals/hashCode which in turn uses the Lists default implementations. Those will in turn call it's children's methods, being the DataSetsRows.

Try using the original methods to see a stack overflow in the test case.

I am not sure about the best implementation of the equals/hashCode methods in this case, please see it just as a suggestion.

wirew0rm
wirew0rm previously approved these changes Jan 26, 2021
Copy link
Member

@wirew0rm wirew0rm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, using identity based equalness should be good here with model being final and initialized with a new Instance for every TableViewer.

@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #343 (a8923d5) into master (aa9be37) will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage   51.77%   51.78%   +0.01%     
==========================================
  Files         394      394              
  Lines       41214    41214              
  Branches     6623     6623              
==========================================
+ Hits        21337    21344       +7     
+ Misses      18364    18358       -6     
+ Partials     1513     1512       -1     
Impacted Files Coverage Δ
...rc/main/java/de/gsi/chart/plugins/TableViewer.java 50.12% <50.00%> (+1.80%) ⬆️
...ava/de/gsi/chart/utils/SimplePerformanceMeter.java 87.09% <0.00%> (-1.62%) ⬇️
...hart/plugins/measurements/DataSetMeasurements.java 75.77% <0.00%> (+0.34%) ⬆️

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 aa9be37...a8923d5. Read the comment docs.

@RalphSteinhagen
Copy link
Member

@dedeibel thanks for the PR! The identity-based comparison is also OK from my point of view. We are missing updated/custom hashCode(), equals(), and meaningful toString() functions here and there. Thus, these types of PR are much appreciated! 👍

N.B. as Obi-Wan once said: "Trust_the clang-formatter" ... and the QA checks for that matter. :-)

Do you think you could upgrade the unit-test to improve the coverage also for this modified diff code?

I am not only a fan of good unit-tests, but experience has shown that these make the code much more reliable as well as easier and more predictable to refactor or modify functionality. And as always, keep up the good work @dedeibel!

@dedeibel dedeibel force-pushed the fix-table-plugin-row-hashcode-equals branch from fab8cfb to a309cd0 Compare January 27, 2021 12:27
@dedeibel dedeibel force-pushed the fix-table-plugin-row-hashcode-equals branch 2 times, most recently from 836130e to 3f28225 Compare February 3, 2021 12:06
@dedeibel dedeibel temporarily deployed to coverage February 3, 2021 12:06 Inactive
@dedeibel dedeibel temporarily deployed to coverage February 3, 2021 12:06 Inactive
@dedeibel dedeibel temporarily deployed to coverage February 3, 2021 12:06 Inactive
@dedeibel dedeibel temporarily deployed to deploy February 3, 2021 12:17 Inactive
@dedeibel dedeibel temporarily deployed to coverage February 3, 2021 12:33 Inactive
@dedeibel dedeibel temporarily deployed to coverage February 3, 2021 12:33 Inactive
@dedeibel dedeibel temporarily deployed to deploy February 3, 2021 12:42 Inactive
@RalphSteinhagen RalphSteinhagen force-pushed the fix-table-plugin-row-hashcode-equals branch from 3f28225 to a8923d5 Compare February 3, 2021 13:53
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage February 3, 2021 13:53 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage February 3, 2021 13:53 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage February 3, 2021 13:53 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to coverage February 3, 2021 13:53 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy February 3, 2021 14:01 Inactive
@RalphSteinhagen RalphSteinhagen temporarily deployed to deploy February 3, 2021 14:03 Inactive
@RalphSteinhagen RalphSteinhagen merged commit ec0da41 into master Feb 3, 2021
@RalphSteinhagen RalphSteinhagen deleted the fix-table-plugin-row-hashcode-equals branch February 3, 2021 14:08
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