Alert user to places where the block text at the time of recording != current block text#16
Alert user to places where the block text at the time of recording != current block text#16tombogle wants to merge 10 commits into
Conversation
…espace changes and actual word changes.
…ces where blocks are out of synch with the text as it was at the time of recording. HT-196: Fixed initial display of block following skipped blocks.
…omments to help with possible future enhancements.
…eing cut off. Also, fixed problem where changing admin settings was changing which block was selected.
…way out of date. It does not currently build. All conflicts and build errors need to be studied carefully! Merge branch 'master' into AlertUserToChangedBlocks # Conflicts: # src/HearThis/HearThis.csproj # src/HearThis/Properties/Resources.Designer.cs # src/HearThis/UI/RecordingToolControl.Designer.cs # src/HearThis/UI/RecordingToolControl.cs # src/HearThis/UI/Shell.cs
JohnThomson
left a comment
There was a problem hiding this comment.
Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tombogle)
src/HearThis/UI/RecordingToolControl.cs, line 420 at r1 (raw file):
_project.SelectedChapterInfo.ChapterNumber1Based, i)) { brushes[iBrush++] = GetIsRecordingInSynchWithText(i) ? AppPallette.HighlightBrush : AppPallette.ButtonRecordingBrush;
I think something is wrong here. iBrush is (usually) an index into results, and the length of results is fixed in advance. I'm not clear why we're putting a value into brushes here instead of setting seg.UnderlineBrush, or why we even might use this index, but incrementing it twice in one iteration seems almost certain to cause a problem. If this is really right, I think we need comments explaining why we'd want to leave a gap in results and why we're using the same variable to index two different arrays and incrementing for each.
src/HearThis/UI/RecordingToolControl.cs, line 768 at r1 (raw file):
var availWidth = _tableLayoutPanelNavigationState.GetColumnWidths()[1] - _warningIcon.Width - _warningIcon.Margin.Horizontal; var g = Graphics.FromHwnd(Handle); if (availWidth > 0 && availWidth > TextRenderer.MeasureText(g, _labelClipOutOfSynchWithBlock.Text, _lineCountLabel.Font).Width)
My instinct is that we should be measuring in the default font to decide whether we can use that or need something else (presumably smaller).
src/HearThis/UI/RecordingToolControl.cs, line 776 at r1 (raw file):
public bool HidingSkippedBlocks { get { return false; } // Currently we never want to do this. Some of the newer code doesn't handle it.
In that case it seems we should either delete (or comment out) the set code, or at least make it assert(false) if something tries to set it true.
src/HearThis/UI/Shell.cs, line 254 at r1 (raw file):
//_recordingToolControl1.HidingSkippedBlocks = Settings.Default.ActiveMode == kNormalRecording; obsolete _recordingToolControl1.ShowingSkipButton = Settings.Default.ActiveMode != kNormalRecording; // TODO: Figure out which version of this we need/want: _recordingToolControl1.HidingAdministrativeControls = SettingsProtectionSingleton.Settings.RequirePassword;
Should this be resolved before merging?
src/HearThis/UI/Shell.cs, line 324 at r1 (raw file):
//_recordingToolControl1.HidingSkippedBlocks = Settings.Default.ActiveMode == kNormalRecording; obsolete _recordingToolControl1.ShowingSkipButton = Settings.Default.ActiveMode != kNormalRecording; // TODO: which version??? _recordingToolControl1.HidingAdministrativeControls = SettingsProtectionSingleton.Settings.RequirePassword;
Should this be resolved before merging?
src/HearThis/Utils/Utils.cs, line 25 at r1 (raw file):
// This regex is intended to match any sequence of spaces or punctuation that is not word medial. It isn't quite right // yet (see commented out AreWordsIdentical tests). static readonly Regex s_replacePunctuationAndWhitespaceRegex = new Regex(@"((^|\s)[^\w']*)|([^\w']*($|\s))", RegexOptions.Compiled);
There are Unicode character class escapes that might be helpful (e.g., to match specifically things Unicode considers punctuation)
|
This has now mostly (if not completely) been superseded by the work on HT-359 and other tasks. Code changes should be reviewed to see if there is anything worth redeeming. |
This PR also includes the fix for HT-195
This document contains an analysis of the the problem and a record of interactions with the analyst:
https://docs.google.com/document/d/1CSwUfGsXLzkZLfv5UTsK4kjbrnq-kDNAQG7_8q7n3tY/edit?pli=1#heading=h.5yzzgi38gyh0
I believe this is closely related to theses Jira issues:
https://jira.sil.org/browse/HT-62
https://jira.sil.org/browse/HT-238
https://jira.sil.org/browse/HT-74
This change is