Reintegrated changes from https://github.com/MultiMC/MultiMC5/tree/develop/libraries/ganalytics

[kropp]

I've integrated changes from https://github.com/MultiMC/MultiMC5/tree/develop/libraries/ganalytics mentioned in #16 Fixed examples, so right now this library can be used both in Qt Creator and CMake

[theoriginalgri]

Hello Victor,

thank you for your pull request and sorry for the delayed answer. I am very happy to see that other people care about this project and want to contribute! We could also talk in person about this pull request on Thursday on the Qt Munich Meetup.

There are some things which we should include, but it seems like there are multiple changes which cancel each other out. The following changes were introduced by MultiMC5 (@peterix):

• CMake support added - ok
• ganalytics.h was moved to include/ - ok
• GAnalytics class got spaces->tabs, all Q_PROPERTY entries removed, functions reordered, LogLevel::None removed, optional QQmlComponent implementation removed - why?
• ganalytics.cpp was moved to src/ - ok
• GAnalytics::Private was renamed to GAnalyticsWorker and moved to ganalytics_worker.cpp - why?
• GAnalytics::setAnonymizeIPs/anonymizeIPs was added - ok
• GAnalytics::setClientID/clientID was added - ok
• GAnalytics::startSending/isSending/isSendingChanged were replaced by isEnabled/enable - why?
• Hardcoded 30 seconds send-interval was dynamicalized to m_timerInterval - ok
• System detection was moved to sys/ and removed - why
• GAnalytics::version and constructor added - why

I would not change the things which I commented with “why?”.

Followed by your changes:

• Remove of sys/ introduced by MultiMC5
• GAnalytics::version constructor parameter removed
• Made compilable without QT_GUI_LIB again
• Merged ganalytics_worker.h back to ganalytics.h
• Added Q_PROPERTY back without changed signals
• Removed QtCreator settings from .gitignore and added cmake

So for me this looks like the author of MultiMC5 changed many things and you reversed some of them to get it working. I propose that we merge the following changes manually (based on our master branch):

• Move files to include/ and src/
• Add CMake configuration
• Add setAnonymizeIPs/anonymizeIPs/anonymizeIPsChanged and setClientID/clientID/clientIDChanged
• Add setEnabled/isEnabled/enabledChanged without removing isSending since this marks "currently working"
• Add version getter
• Add property for send interval
• Add cmake ignores to .gitignore

I can do that, but this would mean we lose the change history from you and MultiMC5. If you want you could make a new pull request (or edit this one) with the above mentioned changes to have a clean and straightforward history. What do you think?

Christoph

[peterix]

I'm sorry that I couldn't get to cleaning that up. There is only so much I can do and the TODO tree is growing, not shrinking :)

Don't worry about the version in MultiMC and feel free to just squash the history away, I do not need my name on the commits.

GAnalytics::Private was renamed to GAnalyticsWorker and moved to ganalytics_worker.cpp - why?

I honestly don't remember. Maybe that made sense at that point.

GAnalytics class got spaces->tabs, all Q_PROPERTY entries removed, functions reordered, LogLevel::None removed, optional QQmlComponent implementation removed - why?

I do not use any of that (properties, QML, the logging in MultiMC is custom).

GAnalytics::startSending/isSending/isSendingChanged were replaced by isEnabled/enable - why?

That was part of a series of changes I ended up throwing away. The name change remained.

The system detection was seriously broken. It:

• did not show up properly in GA for any of the systems
• depended on latest Qt to be able to detect latest operating systems

I still use Qt 5.4.x because of numerous bugs and compatibility issues in newer versions. It would never detect some operating systems properly if I used it for that purpose. First, I tried fixing it, then I decided to just use custom values instead of trying to figure out what to feed to GA...

What I ended up doing with sys: https://github.com/MultiMC/MultiMC5/tree/develop/libraries/systeminfo https://github.com/MultiMC/MultiMC5/blob/develop/application/MultiMC.cpp#L1119

Not ideal, but better than spending an eternity on trying to fake user agent strings... I needed it to work now and keep working ideally forever. User agent strings are not a good way to do that due to their ever-changing nature and lack of standards. Also, I do not target phones of any kind, so I didn't include the phone OS detection. I have no idea if the original works for phones... YMMV.

The purpose of GAnalytics::version is to be able to compare the version of analytics the user agreed with (in external config) and the version in the binary, then show an opt-in/out dialog if they differ. Looking at the way it is used in MultiMC, I could just remove it - the information doesn't have to be passed through the class, because the version number is only used in one method: https://github.com/MultiMC/MultiMC5/blob/develop/application/MultiMC.cpp#L614 It would make more sense if that method was split. If you don't have any use for it, feel free to get rid of it.

On Tue, Jan 24, 2017 at 2:05 PM, Christoph Keller [email protected] wrote:

Hello Victor,

thank you for your pull request and sorry for the delayed answer. I am very happy to see that other people care about this project and want to contribute! We could also talk in person about this pull request on Thursday on the Qt Munich Meetup.

There are some things which we should include, but it seems like there are multiple changes which cancel each other out. The following changes were introduced by MultiMC5 (@peterix https://github.com/peterix):

• CMake support added - ok
• ganalytics.h was moved to include/ - ok
• GAnalytics class got spaces->tabs, all Q_PROPERTY entries removed, functions reordered, LogLevel::None removed, optional QQmlComponent implementation removed - why?
• ganalytics.cpp was moved to src/ - ok
• GAnalytics::Private was renamed to GAnalyticsWorker and moved to ganalytics_worker.cpp - why?
• GAnalytics::setAnonymizeIPs/anonymizeIPs was added - ok
• GAnalytics::setClientID/clientID was added - ok
• GAnalytics::startSending/isSending/isSendingChanged were replaced by isEnabled/enable - why?
• Hardcoded 30 seconds send-interval was dynamicalized to m_timerInterval - ok
• System detection was moved to sys/ and removed - why
• GAnalytics::version and constructor added - why

I would not change the things which I commented with “why?”.

Followed by your changes:

• Remove of sys/ introduced by MultiMC5
• GAnalytics::version constructor parameter removed
• Made compilable without QT_GUI_LIB again
• Merged ganalytics_worker.h back to ganalytics.h
• Added Q_PROPERTY back without changed signals
• Removed QtCreator settings from .gitignore and added cmake

So for me this looks like the author of MultiMC5 changed many things and you reversed some of them to get it working. I propose that we merge the following changes manually (based on our master branch):

• Move files to include/ and src/
• Add CMake configuration
• Add setAnonymizeIPs/anonymizeIPs/anonymizeIPsChanged and setClientID/clientID/clientIDChanged
• Add setEnabled/isEnabled/enabledChanged without removing isSending since this marks "currently working"
• Add version getter
• Add property for send interval
• Add cmake ignores to .gitignore

I can do that, but this would mean we lose the change history from you and MultiMC5. If you want you could make a new pull request (or edit this one) with the above mentioned changes to have a clean and straightforward history. What do you think?

Christoph

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HSAnet/qt-google-analytics/pull/17#issuecomment-274797579, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMaPotyhZu0meBS0D_Rg6p6MudzTAQSks5rVfcCgaJpZM4Lb72F .

[kropp]

Actually, I ended up rewriting Analytics support from scratch due to several reasons. Main are:

• library didn't suit our threading model (e. g. activities are already running in background thread, no need for new one)
• we have our own networking abstractions with a bunch of problems solved. Cost of adapting them was much higher than rewriting. etc.

So, I suggest you'd better merge changes you find useful manually. I don't need my name in commits too. Feel free to rewrite history or close this PR without merging.