Upgraded lots of DLLs and added Analytics events#898
Conversation
Added Analytics events Minor code refactoring
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 39 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tombogle).
Glyssen/MainForm.cs line 1051 at r1 (raw file):
using (var dlg = new AssignCharacterDlg(viewModel)) { TrackAnalyticsForMajorActivity(dlg);
devinreview says
Inconsistent analytics tracking timing across dialog invocations
There's an inconsistency in when TrackAnalyticsForMajorActivity is called:
- In
ShowModalDialogWithWaitCursor(MainForm.cs:354-355): Called AFTERShowDialog()and only if result != Cancel - In
Assign_Click(MainForm.cs:1051): Called BEFOREShowDialog(), unconditionally - In
Settings_Click(MainForm.cs:1151): Called AFTERShowDialog()and only if result == OK
This means Assign_Click will track analytics even if the user immediately cancels, while other dialogs only track on completion. This may be intentional (tracking that the dialog was opened vs. completed), but the inconsistency could affect analytics interpretation.
Glyssen/MainForm.cs line 1118 at r1 (raw file):
var name = (uiElement as Control)?.Name ?? (uiElement as ToolStripItem)?.Name; if (name == null) throw new ArgumentException(nameof(uiElement), "Expected a UI element with a name");
devinreview points out the parameter order is backwards.
Here's an example of a way that AI review is better than human review. I didn't/wouldn't have caught it.
Jettisoned DotNetZip.
Added Analytics events for starting a new project and other major user actions.
Minor code refactoring.
This change is