Analytics tracking enhancements by tombogle · Pull Request #259 · sillsdev/saymore

tombogle · 2026-01-06T14:35:40Z

This change is

…ning them.

Included some minor refactoring and code cleanup

Also, added missing DLL to Installer

Could not load file or assembly 'System.Text.Json, Version=9.0.0.0'

…by tests Added explicit method to call to report project progress. Included advances in completed stages in project progress analytics

andrew-polk

@andrew-polk reviewed 20 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle).

src/SayMore/Model/Project.cs line 208 at r1 (raw file):

					if (personDelta > 0)
						properties["PersonsAdded"] = personDelta.ToString();

I guess you don't care about removal of sessions or persons? (assuming that is possible)

Same for media duration below

Deal with race condition to be able to track progress reliably. Remove unnecessary tracking of intermediate updates.

tombogle

@tombogle made 1 comment.
Reviewable status: 15 of 20 files reviewed, 1 unresolved discussion (waiting on @andrew-polk).

src/SayMore/Model/Project.cs line 208 at r1 (raw file):

Previously, andrew-polk wrote…

I guess you don't care about removal of sessions or persons? (assuming that is possible)

Same for media duration below

I'm thinking of "progress" as advancing toward some (nebulous) "completion point." While getting rid of cruft can also be a form of progress, it's not the main focus. There are several other things that I could take into account, such as archiving, but that is already a concretely trackable event. My hope is to have an "event" that more-or-less answers the question, did the user make measurable progress toward creation of a finished work during this SayMore session, as opposed to just going in and poking around.

andrew-polk · 2026-01-13T16:38:03Z

src/SayMore/Model/Project.cs line 208 at r1 (raw file):

Previously, tombogle (Tom Bogle) wrote…

I'm thinking of "progress" as advancing toward some (nebulous) "completion point." While getting rid of cruft can also be a form of progress, it's not the main focus. There are several other things that I could take into account, such as archiving, but that is already a concretely trackable event. My hope is to have an "event" that more-or-less answers the question, did the user make measurable progress toward creation of a finished work during this SayMore session, as opposed to just going in and poking around.

👍

andrew-polk

@andrew-polk reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tombogle).

src/SayMore/Model/Project.cs line 680 at r2 (raw file):

SetAdditionalMetsData

typo
SetAdditionalMetaData

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot · 2026-01-13T17:08:53Z

+		public void TrackStatistics(StatisticsViewModel statisticsViewModel)
+		{ 
+			if (_statisticsViewModel != null)
+				_statisticsViewModel.FinishedGatheringStatisticsForAllFiles -= FinishedGatheringStatistics;
+
+			_statisticsViewModel = statisticsViewModel;
+			_statisticsViewModel.FinishedGatheringStatisticsForAllFiles += FinishedGatheringStatistics;
+			if (_statisticsViewModel.IsDataUpToDate)
+			{
+				// It either finished before we could hook the event, or we hit the race condition.
+				FinishedGatheringStatistics(_statisticsViewModel, null);
+			}
+		}
+
+		private void FinishedGatheringStatistics(object sender, EventArgs e)
+		{
+			_statisticsViewModel.FinishedGatheringStatisticsForAllFiles -= FinishedGatheringStatistics;
+			// Check for race condition (see above).
+			if (_progressStats != null)
+				return;
+
+			_progressStats = new ProgressStats(_statisticsViewModel);
+		}


The new analytics tracking functionality (TrackStatistics, ReportProgressIfAny, and ProgressStats class) lacks test coverage. Since the repository has comprehensive test coverage for both Project and StatisticsViewModel classes, similar test coverage should be added for these new analytics-related methods to verify correct tracking behavior and edge cases.

Copilot · 2026-01-13T17:08:53Z

+			// Check for race condition (see above).
+			if (_progressStats != null)
+				return;
+
+			_progressStats = new ProgressStats(_statisticsViewModel);


Potential race condition: _progressStats is accessed from multiple contexts (FinishedGatheringStatistics callback, ReportProgressIfAny, and Dispose) without synchronization. While the null check at line 971 helps, a more robust approach would be to use locking or Interlocked operations to ensure thread-safe access to this field, especially since the statistics gatherer runs on a background thread.

Suggested change

// Check for race condition (see above).

if (_progressStats != null)

return;

_progressStats = new ProgressStats(_statisticsViewModel);

// Use atomic initialization to avoid race conditions when multiple threads

// access _progressStats concurrently.

var newProgressStats = new ProgressStats(_statisticsViewModel);

if (Interlocked.CompareExchange(ref _progressStats, newProgressStats, null) != null)

{

// Another thread has already initialized _progressStats; discard this instance.

// If ProgressStats implements IDisposable, it should be disposed here.

}

Copilot

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

src/SayMore/UI/SplashScreenForm.Designer.cs: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-13T18:58:24Z

+		public void TrackStatistics(StatisticsViewModel statisticsViewModel)
+		{ 
+			if (_statisticsViewModel != null)
+				_statisticsViewModel.FinishedGatheringStatisticsForAllFiles -= FinishedGatheringStatistics;
+
+			_statisticsViewModel = statisticsViewModel;
+			_statisticsViewModel.FinishedGatheringStatisticsForAllFiles += FinishedGatheringStatistics;
+			if (_statisticsViewModel.IsDataUpToDate)
+			{
+				// It either finished before we could hook the event, or we hit the race condition.
+				FinishedGatheringStatistics(_statisticsViewModel, null);
+			}
+		}


The new TrackStatistics method and analytics tracking logic lack test coverage. Consider adding tests to verify: 1) TrackStatistics is called when StatisticsViewModel is created, 2) ReportProgressIfAny correctly tracks deltas, 3) Analytics.Track is called with expected properties.

We don't generally test Analytics code. Given the asyncronous nature of the statistics collection, it's probably not worth the trouble to introduce potentially flaky tests.

tombogle

@tombogle made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrew-polk).

src/SayMore/Model/Project.cs line 680 at r2 (raw file):

Previously, andrew-polk wrote…

SetAdditionalMetsData

typo
SetAdditionalMetaData

It really is METS. It's an acronym for Metadata Encoding and Transmission Standard (https://www.loc.gov/standards/mets/)

tombogle · 2026-01-13T19:29:59Z

+		public void TrackStatistics(StatisticsViewModel statisticsViewModel)
+		{ 
+			if (_statisticsViewModel != null)
+				_statisticsViewModel.FinishedGatheringStatisticsForAllFiles -= FinishedGatheringStatistics;
+
+			_statisticsViewModel = statisticsViewModel;
+			_statisticsViewModel.FinishedGatheringStatisticsForAllFiles += FinishedGatheringStatistics;
+			if (_statisticsViewModel.IsDataUpToDate)
+			{
+				// It either finished before we could hook the event, or we hit the race condition.
+				FinishedGatheringStatistics(_statisticsViewModel, null);
+			}
+		}


We don't generally test Analytics code. Given the asyncronous nature of the statistics collection, it's probably not worth the trouble to introduce potentially flaky tests.

andrew-polk

@andrew-polk resolved 1 discussion and dismissed @copilot[bot] from a discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tombogle).

Copilot · 2026-01-13T20:26:15Z

@andrew-polk I've opened a new pull request, #261, to work on those changes. Once the pull request is ready, I'll request review from you.

andrew-polk · 2026-01-13T20:26:57Z

Sigh. I don't know why copilot did that. Closing it.

Updated copyright date in license file Added Crowdin to acknowledgments in aboutBox.htm

WIP: Getting everything to compile...

Exposes the SayMore-specific ILocalizationManager as a static so EditorBase and other classes can subscribe to UiLanguageChanged without needing constructor injection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… constructors - Replace `EditorBase(ILocalizationManager)` with `protected EditorBase()` that reads from `ApplicationContainer.SayMoreLocalizationManager` directly - Replace 4-arg `EditorBase(ComponentFile, string, string, ILocalizationManager)` with 3-arg version delegating to the no-arg constructor - Use null-guard `if (_localizationManager != null)` for event subscription/ unsubscription to support unit tests where ApplicationContainer is not initialized - Fixes "does not contain a constructor that takes 0 arguments" errors in MediaComponentEditor, DiagnosticsFileInfoControl, and related subclasses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ager static Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… static Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…sticsView

Drop ILocalizationManager constructor params and field storage from FileType base, AnnotationFileWithMissingMediaFileType, AudioVideoFileTypeBase, AudioFileType, VideoFileType, OralAnnotationFileType, and ImageFileType. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ubclasses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…call and lm guard Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… in 5 EditorBase subclasses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ojectDocsScreen Drop lm constructor params from ProjectMetadataScreen, ProjectDocsScreen, ProjectDescriptionDocsScreen, and ProjectOtherDocsScreen. Remove _localizationManager field and clean up HandleStringsLocalized and HandleAfterComponentFileSelected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… ShowLocalizationDialogBox with Crowdin URL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… ContributorsEditor) These were not in the original plan but had the same 4-arg base call and lm guard patterns as Task 8 files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…nstructor to OnLoad Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…from EditorBase tabText was always null at every call site; Initialize only set ImageKey (after a no-op TabText assignment). Replace the 3-param EditorBase constructor with a 2-param one that sets ImageKey directly, remove Initialize from both the IEditorProvider interface and the class body, and update all 18 subclasses (including MediaComponentEditor and its two subclasses) to drop the null arg. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… after lm removal)

# Conflicts: # build/TestInstallerBuild.bat

# Conflicts: # src/SayMore/Model/Project.cs # src/SayMore/UI/ProjectChoosingAndCreating/WelcomeDialog.cs

…for the _gender control.

tombogle added 3 commits December 12, 2025 12:20

Added information about projects and events to track creating and ope…

54b5fc4

…ning them.

Merge branch 'main' into analytics-tracking-enhancements

da7147c

Added an event to note details of significant progress.

393b97b

Included some minor refactoring and code cleanup

tombogle requested a review from andrew-polk January 6, 2026 14:35

tombogle self-assigned this Jan 6, 2026

tombogle added 6 commits January 6, 2026 09:50

Fixed problems when stats are not fully initialized

5768024

Upgraded SIL.DesktopAnalytics

edbca9f

Also, added missing DLL to Installer

Merge branch 'main' into analytics-tracking-enhancements

b3a4bf2

Unified on System.Text.Json 9.0.0 to prevent runtime failure:

016fab7

Could not load file or assembly 'System.Text.Json, Version=9.0.0.0'

Improved installer to avoid picking up older DLL versions referenced …

c71c5fd

…by tests Added explicit method to call to report project progress. Included advances in completed stages in project progress analytics

andrew-polk reviewed Jan 13, 2026

View reviewed changes

Revert errant installer change

ff5e863

Deal with race condition to be able to track progress reliably. Remove unnecessary tracking of intermediate updates.

tombogle commented Jan 13, 2026

View reviewed changes

andrew-polk approved these changes Jan 13, 2026

View reviewed changes

andrew-polk requested a review from Copilot January 13, 2026 17:02

Copilot started reviewing on behalf of andrew-polk January 13, 2026 17:03 View session

Copilot AI reviewed Jan 13, 2026

andrew-polk requested a review from Copilot January 13, 2026 18:55

Copilot started reviewing on behalf of andrew-polk January 13, 2026 18:55 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

tombogle commented Jan 13, 2026

View reviewed changes

andrew-polk approved these changes Jan 13, 2026

View reviewed changes

Copilot AI mentioned this pull request Jan 13, 2026

[WIP] Add changes to address feedback on analytics tracking PR #261

Closed

tombogle added 3 commits January 13, 2026 16:02

Used locking to prevent race conditions in statistics reporting.

ba01181

Cleaned up some long-standing compiler warnings

ede6694

Updated copyright date in license file Added Crowdin to acknowledgments in aboutBox.htm

Upgrade libpalaso SIL DLLs and L10nSharp to latest betas.

7563c3b

WIP: Getting everything to compile...

tombogle and others added 20 commits June 5, 2026 11:09

Make ComponentFileGrid constructor no-arg, use SayMoreLocalizationMan…

f105fc4

…ager static Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

StatisticsView/ProgressScreen: remove ILocalizationManager param, use…

5cbfb09

… static Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix indentation on null-conditional UiLanguageChanged guards in Stati…

0df26dd

…sticsView

Task 6: remove lm guard from HandleStringsLocalized in 7 EditorBase s…

05e6a4d

…ubclasses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Task 7: fix PersonBasicEditor — remove no-arg HandleStringsLocalized …

870e9f7

…call and lm guard Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Task 8: drop lm constructor param and clean up HandleStringsLocalized…

72ff644

… in 5 EditorBase subclasses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Task 10: rewrite UILanguageDlg — remove ILocalizationManager, replace…

cc5cb1d

… ShowLocalizationDialogBox with Crowdin URL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Task 11a: fix two missed EditorBase subclasses (BasicFieldGridEditor,…

c3e4e92

… ContributorsEditor) These were not in the original plan but had the same 4-arg base call and lm guard patterns as Task 8 files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Refactor: move HandleStringsLocalized initial call from EditorBase co…

cb3ef52

…nstructor to OnLoad Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Add superpowers/ to .gitignore

39df720

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Fix: defer gender ComboBox selection until items are populated (crash…

928ad45

… after lm removal)

Merge branch 'main' into analytics-tracking-enhancements

a5d78b4

# Conflicts: # build/TestInstallerBuild.bat

Merge branch 'main' into analytics-tracking-enhancements

66fa26d

# Conflicts: # src/SayMore/Model/Project.cs # src/SayMore/UI/ProjectChoosingAndCreating/WelcomeDialog.cs

Fixed problem with unit tests when trying to select an invalid index …

bc0d247

…for the _gender control.

Updated to latest (beta) SIL DLLs

1aa9d31

-			// Check for race condition (see above).
-			if (_progressStats != null)
-				return;
-			_progressStats = new ProgressStats(_statisticsViewModel);
+			// Use atomic initialization to avoid race conditions when multiple threads
+			// access _progressStats concurrently.
+			var newProgressStats = new ProgressStats(_statisticsViewModel);
+			if (Interlocked.CompareExchange(ref _progressStats, newProgressStats, null) != null)
+			{
+				// Another thread has already initialized _progressStats; discard this instance.
+				// If ProgressStats implements IDisposable, it should be disposed here.
+			}

Uh oh!

Uh oh!

Conversation

tombogle commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-polk commented Jan 13, 2026

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

tombogle Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

tombogle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tombogle Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Jan 13, 2026

Uh oh!

andrew-polk commented Jan 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tombogle commented Jan 6, 2026 •

edited

Loading