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

[FEAT] Improve matomoObserver to dispense with TraceableClientMixin. #173

Closed
lsaudon opened this issue Sep 17, 2024 · 7 comments · Fixed by #174
Closed

[FEAT] Improve matomoObserver to dispense with TraceableClientMixin. #173

lsaudon opened this issue Sep 17, 2024 · 7 comments · Fixed by #174
Labels
enhancement New feature or request

Comments

@lsaudon
Copy link
Contributor

lsaudon commented Sep 17, 2024

Is your feature request related to a problem? Please describe.

We can extend of RouteObserver<PageRoute<dynamic>> to dispense with TraceableClientMixin.

Describe the solution you'd like

Proof of concept:

import 'package:flutter/material.dart';
import 'package:matomo_tracker/matomo_tracker.dart';

class MatomoObserver extends RouteObserver<PageRoute<dynamic>> {
  void _trackPageView(final Route<dynamic>? route) {
    MatomoTracker.instance.trackPageViewWithName(
      actionName: route?.settings.name ?? 'unknown',
    );
  }

  @override
  void didPush(
    final Route<dynamic> route,
    final Route<dynamic>? previousRoute,
  ) {
    super.didPush(route, previousRoute);
    _trackPageView(route);
  }

  @override
  void didPop(final Route<dynamic> route, final Route<dynamic>? previousRoute) {
    super.didPop(route, previousRoute);
    _trackPageView(previousRoute);
  }
}
@TesteurManiak TesteurManiak added the enhancement New feature or request label Sep 18, 2024
@TesteurManiak
Copy link
Member

I'm not sure to understand what is the link with TraceableClientMixin. 🤔
Your suggested implementation looks to me more like a global route observer that would track all page navigation in the app, where TraceableClientMixin is made to be used with specific widgets that you want to track in mind.

@lsaudon
Copy link
Contributor Author

lsaudon commented Sep 18, 2024

Yes, that's right.
The goal is just to add the ability to track globally without having to use TraceableClientMixin everywhere.

@TesteurManiak
Copy link
Member

Thanks for the clarification!
I'm wondering if it is worth integrating this additional observer directly into the package, as showcased the implementation is only about 20 lines of code and the usage might be misleading alongside the existing matomoObserver.
Maybe we could add a section in the documentation or an example in the repo with the code snippet you've provided to inform users of this technique to globally track navigation in their app. 🤔
(I'm open to suggestions, would love to have a second opinion on this)

@lsaudon
Copy link
Contributor Author

lsaudon commented Sep 19, 2024

I need to track all page changes.
I think others have this need.
The current solution makes this tedious.
There may be a solution to merge the two.
Track all pages and for certain widgets (pop up, etc.) use TraceableClientMixin.

@TesteurManiak
Copy link
Member

I need to track all page changes.
I think others have this need.

Sure, but there might also be people out there who needs to only track specific page.

The current solution makes this tedious.

And the implementation you've provided is short and doesn't rely on any kind of internal API of the package hence why I'm not sure that it needs to be integrated directly in it.

There may be a solution to merge the two.
Track all pages and for certain widgets (pop up, etc.) use TraceableClientMixin.

As said above some devs might not want to track indiscriminately all pages in their apps, having more granularity in the tracking seems to me as important as providing a global observer. Because of this difference in behavior I'm not sure it is possible to mix the two, if anything is implemented a separate observer dedicated to TraceableClientMixin should be kept.

@lsaudon
Copy link
Contributor Author

lsaudon commented Sep 19, 2024

The observer can be configured to include or exclude path or name page.

The solution I have shown is simplistic for the moment, but it can be improved.

@TesteurManiak
Copy link
Member

The observer can be configured to include or exclude path or name page.

The issue here is that any type of configuration will make the observer incompatible with TraceableClientMixin because it needs to point to specific observer's reference. (hence why the matomoObserver variable)

I think it's preferable to have a MatomoGlobalObserver (that would be instanciated by the dev, so it would be possible to customize some properties) and a matomoLocalObserver (used only by TraceableClientMixin).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants