From 60b024d24e310a3a1ed3ae1014c8ecdaf490a669 Mon Sep 17 00:00:00 2001 From: Andy McFadden Date: Wed, 12 Jan 2022 14:07:30 -0800 Subject: [PATCH] Improve instruction operand edit edge case The instruction operand editor has a shortcut button for editing a project symbol. Attempting to change the comment field twice without closing the operand editor in between would result in a complaint about duplicate symbol names. --- Asm65/Label.cs | 8 +++++-- SourceGen/WpfGui/EditDefSymbol.xaml.cs | 23 ++++++++++++++++--- .../WpfGui/EditInstructionOperand.xaml.cs | 2 +- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/Asm65/Label.cs b/Asm65/Label.cs index 0e7abb0..f5e47ca 100644 --- a/Asm65/Label.cs +++ b/Asm65/Label.cs @@ -32,12 +32,16 @@ namespace Asm65 { /// /// String comparer to use when comparing labels. - /// + /// + /// + /// Usage: + /// if (Label.LABEL_COMPARER.Equals(label1, label2)) { ... } + /// /// We may want case-insensitive string compares, and we want the "invariant culture" /// version for consistent results across users in multiple locales. (The labels are /// expected to be ASCII strings, so the latter isn't crucial unless we change the /// allowed set.) - /// + /// public static readonly StringComparer LABEL_COMPARER = LABELS_CASE_SENSITIVE ? StringComparer.InvariantCulture : StringComparer.InvariantCultureIgnoreCase; diff --git a/SourceGen/WpfGui/EditDefSymbol.xaml.cs b/SourceGen/WpfGui/EditDefSymbol.xaml.cs index 9bc21fb..542a542 100644 --- a/SourceGen/WpfGui/EditDefSymbol.xaml.cs +++ b/SourceGen/WpfGui/EditDefSymbol.xaml.cs @@ -277,10 +277,27 @@ namespace SourceGen.WpfGui { out Symbol.LabelAnnotation unused4); bool labelUnique; - // NOTE: should be using Asm65.Label.LABEL_COMPARER? if (mDefSymbolList.TryGetValue(trimLabel, out DefSymbol existing)) { - // It's okay if it's the same object. - labelUnique = (existing == mOldSym); + // We found a match. See if we're just seeing the symbol we're editing. + // + // We only want to check the label, not the entire symbol, because otherwise + // things can go funny when multiple edits are done without flushing the data + // back to the symbol table. For example, when this is invoked from the + // Edit Project Symbol button in the instruction operand editor, the user might + // edit the comment field of an existing project symbol, hit OK here, then try + // to edit it again before closing the operand editor. In that case we get + // passed the edited DefSymbol, which no longer fully matches what's in the + // symbol table. + // + // TODO: we still don't handle the case where the user changes the label from + // FOO to FOO1 and then back to FOO without closing the instruction edit dialog. + // The problem is that we find a match for FOO in the symbol table without + // realizing that it was the original name before the edits began. To fix this + // we need to pass in the original label as well as the recently-edited symbol, + // and allow the new name to match either. + + //labelUnique = (existing == mOldSym); + labelUnique = Asm65.Label.LABEL_COMPARER.Equals(existing.Label, mOldSym.Label); } else { labelUnique = true; } diff --git a/SourceGen/WpfGui/EditInstructionOperand.xaml.cs b/SourceGen/WpfGui/EditInstructionOperand.xaml.cs index 4c92773..83adc58 100644 --- a/SourceGen/WpfGui/EditInstructionOperand.xaml.cs +++ b/SourceGen/WpfGui/EditInstructionOperand.xaml.cs @@ -917,7 +917,7 @@ namespace SourceGen.WpfGui { // which will traverse multiple items to find a match. // TODO: this can create a situation where the code list shows FUBAR-1 but we // edit an earlier label, if the earlier label has a multi-byte format that - // includes the target address. (An example can be found in 2024-ui-edge-cases.) + // includes the target address. (An example can be found in 20200-ui-edge-cases.) mEditedLabelOffset = DataAnalysis.GetBaseOperandOffset(mProject, attr.OperandOffset); mLabelTargetAddress = mProject.GetAnattrib(mEditedLabelOffset).Address;