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

Add CalculationListener.calculationsStarted #1455

Merged
merged 2 commits into from
Dec 22, 2016

Conversation

cjkent
Copy link
Contributor

@cjkent cjkent commented Dec 21, 2016

Adds a method to CalculationListener that is invoked before any results are calculated. This allows the listener to know about the columns without the user having to pass the columns to both the listener and the calculation runner.

@@ -32,16 +32,16 @@
private final List<CalculationResult> results = new ArrayList<>();

/** The columns that define what values are calculated. */
private final List<Column> columns;
private List<Column> columns;
Copy link
Member

Choose a reason for hiding this comment

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

volatile? Since the listener may be called from different threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The calling code goes to great lengths to ensure everything is synchronised so all changes are visible to all threads. The Javadoc on CalculationListener specifies that implementations don't have to be thread safe.

*
* @param columns the columns for which values are being calculated
*/
public default void calculationsStarted(List<Column> columns) {
Copy link
Member

Choose a reason for hiding this comment

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

Take list of targets as well as columns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems reasonable. Our implementations won't use it but there's no harm adding it and I can see why it might be useful. It's also available at the call site so it's easy to add.

Copy link
Member

@jodastephen jodastephen 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

@jodastephen jodastephen merged commit 0805c27 into master Dec 22, 2016
@jodastephen jodastephen deleted the topic/calculations-started branch December 22, 2016 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants