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

Update LinkTargetIdFeature.java #257

Merged
merged 1 commit into from
Sep 1, 2023
Merged

Update LinkTargetIdFeature.java #257

merged 1 commit into from
Sep 1, 2023

Conversation

stefanhahmann
Copy link
Collaborator

Resolves: #217

Replace HashSet used as return value for the projections() method by LinkedHashSet, since the order of entry needs to be stable in process using this method.

Replace HashSet used for projections() by LinkedHashSet, since the order of entry needs to be stable in process using this method.
@tinevez tinevez merged commit 82eb80c into dev Sep 1, 2023
1 check passed
@stefanhahmann
Copy link
Collaborator Author

stefanhahmann commented Sep 1, 2023

@tinevez Since the order of implementations of the org.mastodon.feature.Feature#projections() method needs to be reproducible, do you think, it would be good to change the return value of the method from Set to LinkedHashSet or alternatively to List? By this, errors like the one fixed with this PR could not happen again by people implementing this interface method who do not know that the order is relevant in some cases.

cf.

public Set< FeatureProjection< T > > projections();

@tinevez
Copy link
Contributor

tinevez commented Sep 4, 2023

@stefanhahmann Actually the order in which projections are returned should not matter.
I think there is a deeper problem, probably rooted in the Table headers

@tinevez tinevez deleted the stefanhahmann-patch-1 branch September 4, 2023 12:33
@stefanhahmann
Copy link
Collaborator Author

@stefanhahmann Actually the order in which projections are returned should not matter. I think there is a deeper problem, probably rooted in the Table headers

I agree. When fixing this bug, I was also assuming that there is something in table headers which does not appropriately handle the existing projections and thus relies on some order. However, I did not dig it down.

@tinevez
Copy link
Contributor

tinevez commented Sep 8, 2023

I think that the issue is related to the fact that everytime we call the projections() method in this feature, it returns a new HashSet, with new instances of the feature projection classes.
But in the table, when building the header, the projections() method is called twice, once for the header subtitle and once to store the projection values. I think it comes from that.

Your patch fix the issue because it imposes the order. But I will try to fix the table so that the header and values are built from the same instance of the projection, so that we don't depend on order.

But also, I think it is sometimes nicer to impose a column order, like you did :)

@tinevez
Copy link
Contributor

tinevez commented Sep 8, 2023

See dca0534

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.

2 participants