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

change google analytics interface #521

Closed
wants to merge 27 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
import com.google.web.bindery.event.shared.SimpleEventBus;
import com.gwtplatform.common.client.CommonGinModule;
import com.gwtplatform.mvp.client.RootPresenter;
import com.gwtplatform.mvp.client.googleanalytics.GoogleAnalytics;
import com.gwtplatform.mvp.client.googleanalytics.GoogleAnalyticsImpl;
import com.gwtplatform.mvp.client.proxy.DefaultPlaceManager;
import com.gwtplatform.mvp.client.proxy.PlaceManager;
import com.gwtplatform.mvp.shared.proxy.ParameterTokenFormatter;
Expand All @@ -37,10 +35,6 @@
* binding the different classes to their default implementation.
*/
public class DefaultModule extends AbstractGinModule {
private final Class<? extends PlaceManager> placeManagerClass;

private final Class<? extends TokenFormatter> tokenFormatterClass;

/**
* A DefaultModule builder.
*/
Expand All @@ -51,21 +45,25 @@ public static class Builder {
public Builder() {
}

public Builder placeManager(Class<? extends PlaceManager> placeManagerClass) {
public DefaultModule build() {
return new DefaultModule(this);
}

public Builder placeManager(final Class<? extends PlaceManager> placeManagerClass) {
this.placeManagerClass = placeManagerClass;
return this;
}

public Builder tokenFormatter(Class<? extends TokenFormatter> tokenFormatterClass) {
public Builder tokenFormatter(final Class<? extends TokenFormatter> tokenFormatterClass) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you should add "final" in method.

I would like to hear what is your train of though for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry my eclipse adds finally to everything automatically. The reason to add it to methods is it shows instantly that the parameter won't be modified. But I'm happy to remove it if it bothers people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the decision on final parameters. The checkstyle config disallows parameter assignment so I think adding final modifier is an obvious way to enforce that.

this.tokenFormatterClass = tokenFormatterClass;
return this;
}

public DefaultModule build() {
return new DefaultModule(this);
}
}

private final Class<? extends PlaceManager> placeManagerClass;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, line 64 could be removed a swell


private final Class<? extends TokenFormatter> tokenFormatterClass;

/**
* When instantiating the module this way be sure to read
* {@link DefaultPlaceManager}
Expand All @@ -83,6 +81,11 @@ public DefaultModule() {
this(new Builder());
}

private DefaultModule(final Builder builder) {
this.placeManagerClass = builder.placeManagerClass;
this.tokenFormatterClass = builder.tokenFormatterClass;
}

/**
* Manually setup a PlaceManager. See {@link DefaultPlaceManager} for more
* details.<br/>
Expand All @@ -95,7 +98,7 @@ public DefaultModule() {
* @deprecated Please use the {@link com.gwtplatform.mvp.client.gin.DefaultModule.Builder}.
*/
@Deprecated
public DefaultModule(Class<? extends PlaceManager> placeManagerClass) {
public DefaultModule(final Class<? extends PlaceManager> placeManagerClass) {
this(placeManagerClass, ParameterTokenFormatter.class);
}

Expand All @@ -113,25 +116,19 @@ public DefaultModule(Class<? extends PlaceManager> placeManagerClass) {
* @deprecated Please use the {@link com.gwtplatform.mvp.client.gin.DefaultModule.Builder}.
*/
@Deprecated
public DefaultModule(Class<? extends PlaceManager> placeManagerClass,
Class<? extends TokenFormatter> tokenFormatterClass) {
public DefaultModule(final Class<? extends PlaceManager> placeManagerClass,
final Class<? extends TokenFormatter> tokenFormatterClass) {
this.placeManagerClass = placeManagerClass;
this.tokenFormatterClass = tokenFormatterClass;
}

private DefaultModule(Builder builder) {
this.placeManagerClass = builder.placeManagerClass;
this.tokenFormatterClass = builder.tokenFormatterClass;
}

@Override
protected void configure() {
install(new CommonGinModule());

bind(EventBus.class).to(SimpleEventBus.class).in(Singleton.class);
bind(TokenFormatter.class).to(tokenFormatterClass).in(Singleton.class);
bind(RootPresenter.class).asEagerSingleton();
bind(GoogleAnalytics.class).to(GoogleAnalyticsImpl.class).in(Singleton.class);
bind(PlaceManager.class).to(placeManagerClass).in(Singleton.class);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* the License.
*/

package com.gwtplatform.mvp.client.annotations;
package com.gwtplatform.mvp.client.googleanalytics;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,6 @@
*/
public interface GoogleAnalytics {

/**
* Initializes the script for Google Analytics for a specific userAccount.
*
* @param userAccount The Google Analytics account. (i.e. {@code UA-12345678-1})
*/
void init(String userAccount);

/**
* Given that you already have initialized your default Google account with
* {@link #init(String)}, this function will add a new account on which you'll
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,20 @@
import com.google.gwt.dom.client.Element;
import com.google.gwt.dom.client.ScriptElement;
import com.google.gwt.user.client.Window;
import com.google.inject.Inject;

/**
* Default {@link GoogleAnalytics} implementation that uses JSNI to
* expose Google Analytics javascript methods.
*/
public class GoogleAnalyticsImpl implements GoogleAnalytics {
@Override
public void init(String userAccount) {

@Inject
GoogleAnalyticsImpl(@GaAccount String userAccount) {
init(userAccount);
}

private void init(String userAccount) {
Element firstScript = Document.get().getElementsByTagName("script").getItem(
0);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@

import javax.inject.Inject;

import com.google.gwt.core.client.Scheduler;
import com.google.gwt.core.client.Scheduler.ScheduledCommand;
import com.google.web.bindery.event.shared.EventBus;
import com.gwtplatform.mvp.client.annotations.GaAccount;
import com.gwtplatform.mvp.client.proxy.NavigationEvent;
import com.gwtplatform.mvp.client.proxy.NavigationHandler;
import com.gwtplatform.mvp.client.proxy.PlaceManager;
Expand All @@ -38,38 +35,20 @@
* If you want to log custom events, see {@link GoogleAnalytics}.
*/
public class GoogleAnalyticsNavigationTracker implements NavigationHandler {
private final String gaAccount;
private final PlaceManager placeManager;
private final EventBus eventBus;
private final GoogleAnalytics analytics;

@Inject
GoogleAnalyticsNavigationTracker(@GaAccount String gaAccount,
PlaceManager placeManager,
EventBus eventBus,
GoogleAnalytics analytics) {
this.gaAccount = gaAccount;
GoogleAnalyticsNavigationTracker(final PlaceManager placeManager, final EventBus eventBus,
Copy link
Member

Choose a reason for hiding this comment

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

One parameter per line in Ctors (Easier to code review afterward. No need to use intelliJ aligment like before

final GoogleAnalytics analytics) {
this.placeManager = placeManager;
this.eventBus = eventBus;
this.analytics = analytics;

Scheduler.get().scheduleDeferred(new ScheduledCommand() {
@Override
public void execute() {
init();
}
});
}

private void init() {
analytics.init(gaAccount);

eventBus.addHandler(NavigationEvent.getType(), this);
}

@Override
public void onNavigation(NavigationEvent navigationEvent) {
String historyToken = placeManager.buildHistoryToken(navigationEvent.getRequest());
public void onNavigation(final NavigationEvent navigationEvent) {
final String historyToken = placeManager.buildHistoryToken(navigationEvent.getRequest());
analytics.trackPageview(historyToken);
}
}