sillsdev / DesktopAnalytics.net

Wrapper around segment.io's Analytics.net specifically for desktop apps (instead of servers).
5 stars 6 forks source link

Switch from Analytics package to new Analytics.CSharp package #29

Closed tombogle closed 1 year ago

tombogle commented 2 years ago

This fixes deadlock bug properly. This needs to be checked carefully to make sure all the correct properties are coming through in the same way (or that we're okay with any changes). The interfaces changed quite a bit.


This change is Reviewable

jasonleenaylor commented 1 year ago

src/DesktopAnalytics/StatisticsMonitor.cs line 23 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…
It's usually considered best practice (for avoiding deadlocks, I think) to lock on a private object rather than on 'this', which code we don't control could also lock.

Yes, this is accurate. A private locking object in this class would be better.

tombogle commented 1 year ago

Sorry, that change didn't make it in. It's there now. (I only just requested your review today because it had been sitting for a long time unreviewed by the others I had requested. Because of @hatton 's history with this library, I would really want him to review it, but I know he doesn't come up for air very often.)

tombogle commented 1 year ago

src/DesktopAnalytics/Analytics.cs line 594 at r6 (raw file):

Previously, JohnThomson (John Thomson) wrote…
It's a little surprising that we don't do something about _singleton. Seems reasonable that once we've disposed of (presumably) the singleton, we could start over if we wanted, though maybe no one does? At least, would setting it to null cause us to fail faster or more clearly if anyone still tries to send analytics?

Not sure whether it would actually work to try to restart. I see that we check in the constructor to make sure that the singleton is null and throw an exception if not, so that should result in an adequately fast failure. If we were to set it null in Dispose and we really can't restart, then it might fail in a less predictable or understandable way. At least until someone comes up with a scenario where restarting seems desirable and logical, I'm inclined to leave it as is. At least the intended use is clear as it is.

tombogle commented 1 year ago

src/DesktopAnalytics/Analytics.cs line 418 at r6 (raw file):

Previously, JohnThomson (John Thomson) wrote…
It's not a new problem, but the very poor name of this variable makes reading the subsequent code difficult. Wants to be something like "cantGetIpAddress"

Done.

tombogle commented 1 year ago

src/DesktopAnalytics/Properties/AssemblyInfo1.cs line 20 at r6 (raw file):

Previously, JohnThomson (John Thomson) wrote…
Will something maintain this somehow? Otherwise, it seems unfortunate that it is hard-coded that only one exact build of tests can access the internals.

Reverted