Analytics tracking enhancements

[tombogle]

This change is 

[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

[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]

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]

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]

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]

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.

[tombogle]

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]

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]

@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]

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

[tombogle]

@andrew-polk You can re-look at this now or wait. There's (at least) one more small defect to address, but I'm out of time an tokens for now.

[tombogle]

On flag from Devin that I'm not addressing in code but is worth a comment:
src/SayMore/UI/ComponentEditors/PersonBasicEditor.cs:R880-895

EditorBase constructor calls virtual HandleStringsLocalized moved to OnLoad

Previously EditorBase() called HandleStringsLocalized(null) in the constructor; now it subscribes to _localizationManager.UiLanguageChanged and the initial localization is performed in OnLoad (src/SayMore/UI/ComponentEditors/EditorBase.cs:173). Several derived classes still call HandleStringsLocalized(null, EventArgs.Empty) explicitly from their own constructors (e.g. MissingMediaFileEditor, AudioVideoPlayer via FinishInitializing). PersonBasicEditor removed its constructor call to HandleStringsLocalized(null) and relies on OnLoad. Worth confirming that gender combo population (which now happens in OnLoad via HandleStringsLocalized and in HandleBinderTranslateBoundValueBeingRetrieved via the _pendingSelectedGenderIndex deferral) still yields the correct selected gender for the first person loaded, since binding retrieval can happen before OnLoad. The new _pendingSelectedGenderIndex mechanism appears designed exactly for this ordering, but it's a subtle interaction worth a targeted test.

RESPONSE:
I traced this all the way through rather than just eyeballing it, since it's exactly the same mechanism that had a real bug in the sibling finding you already had me fix. This one, though, checks out — no bug.

The key fact: PersonBasicEditor.Designer.cs never adds any items to _gender.Items in InitializeComponent() (confirmed by grep — the designer only sets DropDownStyle, binding flags, size/location, never .Items.Add(...)). So _gender.Items.Count is guaranteed to be 0 at the moment the constructor runs _binder.SetComponentFile(file).

Tracing the sequence for the first person loaded:

Constructor calls _binder.SetComponentFile(file) → fires TranslateBoundValueBeingRetrieved → HandleBinderTranslateBoundValueBeingRetrieved sees _gender.Items.Count == 0 < 2 → takes the defer branch, sets _pendingSelectedGenderIndex. (This is the same if/else I just restored for the person-switching bug — it correctly takes the other branch here.)
Control gets added to its parent; WinForms guarantees OnLoad fires strictly after the constructor returns, on the same UI thread — there's no background-thread race here the way there was in the Project.cs finding, so ordering is deterministic, not probabilistic.
EditorBase.OnLoad → HandleStringsLocalized → populates the two items and applies _pendingSelectedGenderIndex → correct gender shown for the first person.
So the _pendingSelectedGenderIndex mechanism is doing exactly what Devin speculated it was designed for, and the ordering guarantee comes from WinForms' control lifecycle (constructor always completes before OnLoad can fire), not from luck. No code change needed here — I didn't add a speculative test for it since there's nothing broken to pin down, and this codebase doesn't have infrastructure for instantiating PersonBasicEditor with real controls in a unit test (confirmed when I looked into this for the sibling bug).