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

Only load the telementry service as a background task if used #3085

Merged
merged 2 commits into from
Aug 7, 2017

Conversation

chochreiner
Copy link
Collaborator

Up to now, the telemetry service was always running (even if the reporting was turned of).
This initialization already requires a lot of time during startup, which is already rather long on MacOS

I hope I have caught all usages of the telemetry client to avoid nullpointer exceptions, but i would be great if @lynyus could have a look at it.

  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)
  • If you changed the localization: Did you run gradle localizationUpdate?

@chochreiner chochreiner added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 7, 2017
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Besides minor comments: LGTM

@@ -54,6 +54,8 @@
}

private <T extends JabRefDialog> void trackDialogOpening(Class<T> clazz) {
Globals.getTelemetryClient().trackPageView(clazz.getName());
if (Globals.getTelemetryClient()!=null) {
Copy link
Member

Choose a reason for hiding this comment

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

Checkstyle will hit you! 😈

I think, this should be converted to an Optional, shouldn't it? Then the usual Globals.getTelemtryClient().ifPresent(client -> client....)

@LinusDietz
Copy link
Member

@chochreiner my macbook has not yet arrived , so I cant test it on MacOs.

@chochreiner
Copy link
Collaborator Author

The changes work fine on MacOS. I've just added you, bc you have added the telemetry part and I might have overlooked sth, which should also use the "null" checks.

@Siedlerchr
Copy link
Member

The failing tests seem to be about the Help pages, otherwise LGTM

@koppor
Copy link
Member

koppor commented Aug 7, 2017 via email

telemetryClient.flush();
if (Globals.prefs.shouldCollectTelemetry()) {
getTelemetryClient().ifPresent(client -> client.trackSessionState(SessionState.End));
getTelemetryClient().ifPresent(client -> client.flush());
Copy link
Member

@Siedlerchr Siedlerchr Aug 7, 2017

Choose a reason for hiding this comment

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

If you have only a single method call with zero or one parameter, you can do some syntactic sugar by doing ifPresent(TelementryClient::flush)
just for information ;)
https://docs.oracle.com/javase/tutorial/java/javaOO/methodreferences.html

@Siedlerchr Siedlerchr merged commit 5ef7d68 into master Aug 7, 2017
@Siedlerchr Siedlerchr deleted the performance branch August 7, 2017 20:10
@stefan-kolb stefan-kolb requested review from tobiasdiez and removed request for tobiasdiez August 8, 2017 13:01
Siedlerchr added a commit that referenced this pull request Aug 9, 2017
* upstream/master:
  Fix #3034: Make font size in entry editor and group panel customizable (#3083)
  Only load the telementry service as a background task if used (#3085)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants