Skip to content

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

Merged
Siedlerchr merged 2 commits into
masterfrom
performance
Aug 7, 2017
Merged

Only load the telementry service as a background task if used#3085
Siedlerchr merged 2 commits into
masterfrom
performance

Conversation

@chochreiner

Copy link
Copy Markdown
Contributor

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

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Besides minor comments: LGTM


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

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@chochreiner

Copy link
Copy Markdown
Contributor 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
Copy Markdown
Member

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

@koppor

koppor commented Aug 7, 2017 via email

Copy link
Copy Markdown
Member

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

@Siedlerchr Siedlerchr Aug 7, 2017

Copy link
Copy Markdown
Member

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