Skip to content

Commit 0a4e8e1

Browse files
Fix three defects found reviewing the sample index exporter
Resolve option bindings whose path carries a cast. x:Bind spells a cast as a parenthesised type ahead of the path, as in (x:Int32)Columns. Reading the cast as part of the path made the option lookup miss while the whole-word scan still matched, so a binding with a perfectly readable default was treated as having none and its attribute was removed. UniformGrid lost its spans and first column, which is what that sample exists to demonstrate. An attached property path such as (Grid.Row) is a path rather than a cast and is left alone. Stop reading commented-out markup as live. A comment ends at --> and a CDATA section at ]]>, not at the first >. Three scans stopped at the first one and resumed inside the comment, so an x:Name in a comment counted as a declared element, an option binding in a comment was rewritten in the published text, and a prefix used only in a comment was published as a required import. An apostrophe in comment prose also opened a quote that never closed, abandoning prefix stripping for the rest of the fragment. Three samples carry comments of the shape that triggers this today, and are unaffected only because none of them happens to contain a name or a binding. Report a duplicate entry id instead of renaming one. The bare slug went to whichever document was read first and the loser was qualified with its component name, so adding a component that sorted earlier would silently change the id of an entry that had already shipped. An id is published as stable, so a collision is now an error a contributor settles by renaming, and every existing id stays where it is. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 3de2a03 commit 0a4e8e1

5 files changed

Lines changed: 277 additions & 33 deletions

File tree

‎catalog/toolkit-samples.json‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3096,20 +3096,22 @@
30963096
{
30973097
"header": "UniformGrid",
30983098
"details": "Each cell in the grid, by default, will be the same size. If no value for Rows and Columns are provided, the UniformGrid will create a square layout based on the total number of visible items. If a fixed size is provided for Rows and Columns then additional children that can\u0027t fit in the number of cells provided won\u0027t be displayed. In addition, UniformGrid is a Panel instead of an ItemsControl. As such, it could be used as a Panel in such ItemsControls. The UniformGrid inherits from Grid and provides many additional features compared to its predecessor, see more below.",
3099-
"xaml": "\u003CGrid\u003E\n \u003Ccontrols:UniformGrid\u003E\n\n \u003CBorder Grid.Row=\u00221\u0022\n Grid.Column=\u00221\u0022\n Background=\u0022AliceBlue\u0022\u003E\n \u003CTextBlock Text=\u00221\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Cornsilk\u0022\u003E\n \u003CTextBlock Text=\u00222\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022DarkSalmon\u0022\u003E\n \u003CTextBlock Text=\u00223\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Gainsboro\u0022\u003E\n \u003CTextBlock Text=\u00224\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022LightBlue\u0022\u003E\n \u003CTextBlock Text=\u00225\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022MediumAquamarine\u0022\u003E\n \u003CTextBlock Text=\u00226\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022MistyRose\u0022\u003E\n \u003CTextBlock Text=\u00227\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022LightCyan\u0022\u003E\n \u003CTextBlock Text=\u00228\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Salmon\u0022\u003E\n \u003CTextBlock Text=\u00229\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Goldenrod\u0022\u003E\n \u003CTextBlock Text=\u002210\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Pink\u0022\u003E\n \u003CTextBlock Text=\u002211\u0022 /\u003E\n \u003C/Border\u003E\n \u003C/controls:UniformGrid\u003E\n\u003C/Grid\u003E",
3099+
"xaml": "\u003CGrid\u003E\n \u003Ccontrols:UniformGrid Columns=\u00220\u0022\n FirstColumn=\u00221\u0022\n Rows=\u00220\u0022\u003E\n\n \u003CBorder Grid.Row=\u00221\u0022\n Grid.RowSpan=\u00222\u0022\n Grid.Column=\u00221\u0022\n Grid.ColumnSpan=\u00222\u0022\n Background=\u0022AliceBlue\u0022\u003E\n \u003CTextBlock Text=\u00221\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Cornsilk\u0022\u003E\n \u003CTextBlock Text=\u00222\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022DarkSalmon\u0022\u003E\n \u003CTextBlock Text=\u00223\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Gainsboro\u0022\u003E\n \u003CTextBlock Text=\u00224\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022LightBlue\u0022\u003E\n \u003CTextBlock Text=\u00225\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022MediumAquamarine\u0022\u003E\n \u003CTextBlock Text=\u00226\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022MistyRose\u0022\u003E\n \u003CTextBlock Text=\u00227\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022LightCyan\u0022\u003E\n \u003CTextBlock Text=\u00228\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Salmon\u0022\u003E\n \u003CTextBlock Text=\u00229\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Goldenrod\u0022\u003E\n \u003CTextBlock Text=\u002210\u0022 /\u003E\n \u003C/Border\u003E\n \u003CBorder Background=\u0022Pink\u0022\u003E\n \u003CTextBlock Text=\u002211\u0022 /\u003E\n \u003C/Border\u003E\n \u003C/controls:UniformGrid\u003E\n\u003C/Grid\u003E",
31003100
"xmlnsImports": [
31013101
"xmlns:controls=\u0022using:CommunityToolkit.WinUI.Controls\u0022"
31023102
],
31033103
"toolkit": {
31043104
"sampleId": "UniformGridSample",
31053105
"sourcePath": "components/Primitives/samples/UniformGridSample.xaml",
3106+
"optionsResolved": [
3107+
"Columns=0",
3108+
"FirstColumn=1",
3109+
"Item1ColumnSpan=2",
3110+
"Item1RowSpan=2",
3111+
"Rows=0"
3112+
],
31063113
"optionBindingsDropped": [
3107-
"Columns",
3108-
"FirstColumn",
3109-
"Grid.ColumnSpan",
3110-
"Grid.RowSpan",
3111-
"Orientation",
3112-
"Rows"
3114+
"Orientation"
31133115
]
31143116
}
31153117
}
@@ -3239,7 +3241,7 @@
32393241
{
32403242
"header": "RadialGauge",
32413243
"details": "This control will make data visualizations and dashboards more engaging with rich style and interactivity. The round gauges are powerful, easy to use, and highly configurable to present dashboards capable of displaying clocks, industrial panels, automotive dashboards, and even aircraft cockpits. The Radial Gauge supports animated transitions between configuration states. The control gradually animates as it redraws changes to the needle, needle position, scale range, color range, and more.",
3242-
"xaml": "\u003CStackPanel HorizontalAlignment=\u0022Center\u0022\n VerticalAlignment=\u0022Center\u0022\n Orientation=\u0022Horizontal\u0022\u003E\n \u003Ccontrols:RadialGauge x:Name=\u0022RadialGauge\u0022\n Width=\u0022280\u0022\n IsEnabled=\u0022True\u0022\n IsInteractive=\u0022True\u0022\n Maximum=\u0022240\u0022\n Minimum=\u00220\u0022\n NeedleLength=\u002260\u0022\n NeedleWidth=\u00224\u0022\n ScalePadding=\u00220\u0022\n ScaleTickWidth=\u00220\u0022\n ScaleWidth=\u002212\u0022\n StepSize=\u002230\u0022\n TickLength=\u00226\u0022\n TickPadding=\u002224\u0022\n TickWidth=\u00222\u0022\n ValueStringFormat=\u0022N0\u0022\n Value=\u0022120\u0022 /\u003E\n\u003C/StackPanel\u003E",
3244+
"xaml": "\u003CStackPanel HorizontalAlignment=\u0022Center\u0022\n VerticalAlignment=\u0022Center\u0022\n Orientation=\u0022Horizontal\u0022\u003E\n \u003Ccontrols:RadialGauge x:Name=\u0022RadialGauge\u0022\n Width=\u0022280\u0022\n IsEnabled=\u0022True\u0022\n IsInteractive=\u0022True\u0022\n MaxAngle=\u0022150\u0022\n Maximum=\u0022240\u0022\n MinAngle=\u0022-150\u0022\n Minimum=\u00220\u0022\n NeedleLength=\u002260\u0022\n NeedleWidth=\u00224\u0022\n ScalePadding=\u00220\u0022\n ScaleTickWidth=\u00220\u0022\n ScaleWidth=\u002212\u0022\n StepSize=\u002230\u0022\n TickLength=\u00226\u0022\n TickPadding=\u002224\u0022\n TickSpacing=\u002215\u0022\n TickWidth=\u00222\u0022\n ValueStringFormat=\u0022N0\u0022\n Value=\u0022120\u0022 /\u003E\n\u003C/StackPanel\u003E",
32433245
"xmlnsImports": [
32443246
"xmlns:controls=\u0022using:CommunityToolkit.WinUI.Controls\u0022"
32453247
],
@@ -3249,6 +3251,8 @@
32493251
"optionsResolved": [
32503252
"Enabled=True",
32513253
"IsInteractive=True",
3254+
"MaxAngle=150",
3255+
"MinAngle=-150",
32523256
"NeedleLength=60",
32533257
"NeedleWidth=4",
32543258
"ScalePadding=0",
@@ -3257,13 +3261,9 @@
32573261
"StepSize=30",
32583262
"TickLength=6",
32593263
"TickPadding=24",
3264+
"TickSpacing=15",
32603265
"TickWidth=2",
32613266
"Value=120"
3262-
],
3263-
"optionBindingsDropped": [
3264-
"MaxAngle",
3265-
"MinAngle",
3266-
"TickSpacing"
32673267
]
32683268
}
32693269
}

‎tools/SampleIndexExporter.Tests/ContractConformanceTests.cs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33
// See the LICENSE file in the project root for more information.
44

5+
using System.Text.RegularExpressions;
56
using Microsoft.VisualStudio.TestTools.UnitTesting;
67

78
namespace CommunityToolkit.SampleIndex.Tests;
@@ -41,6 +42,29 @@ public void EntryIdsAreUniqueAndUrlSafe()
4142
"Entry ids must be lowercase letters, digits and hyphens:\n " + string.Join("\n ", unsafeIds));
4243
}
4344

45+
[TestMethod]
46+
public void EntryIdDependsOnlyOnItsOwnDocumentFileName()
47+
{
48+
// An id is published as stable, so it has to be a function of the entry's own document
49+
// and nothing else. Deriving it from the set of entries present — qualifying whichever
50+
// of two colliding file names happened to be read second — would silently rename an
51+
// entry that already shipped the day an unrelated component was added. A collision is
52+
// reported as an error instead, so this invariant holds by construction.
53+
var unexpected = RepositoryIndex.Index.Controls
54+
.Where(c => c.Toolkit?.DocumentPath is { } path && c.Id != ExpectedId(path))
55+
.Select(c => $"{c.Id} (expected '{ExpectedId(c.Toolkit!.DocumentPath!)}' from {c.Toolkit!.DocumentPath})")
56+
.ToList();
57+
58+
Assert.AreEqual(
59+
0,
60+
unexpected.Count,
61+
"Entry ids must be derived from their own documentation file name alone:\n "
62+
+ string.Join("\n ", unexpected));
63+
}
64+
65+
private static string ExpectedId(string documentPath) =>
66+
Regex.Replace(Path.GetFileNameWithoutExtension(documentPath).ToLowerInvariant(), "[^a-z0-9]+", "-").Trim('-');
67+
4468
[TestMethod]
4569
public void EveryEntryHasANameAndSourceDocument()
4670
{

‎tools/SampleIndexExporter.Tests/XamlFragmentTests.cs‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,109 @@ public void PageResourcesOpenUpASelfClosingHostElement()
233233
StringAssert.Contains(result.Xaml!, "</Border>");
234234
}
235235

236+
[TestMethod]
237+
public void ACastInABindingPathDoesNotHideTheOption()
238+
{
239+
// x:Bind writes a cast as a parenthesised type ahead of the path. Reading the cast as
240+
// part of the path made the lookup miss, so a binding with a readable default was
241+
// treated as having none and UniformGrid published without its spans or first column.
242+
var result = Extract(
243+
"<Border Grid.RowSpan=\"{x:Bind (x:Int32)Item1RowSpan, Mode=OneWay}\" />",
244+
new SampleOption("Item1RowSpan", "2"));
245+
246+
Assert.IsNull(result.Error);
247+
StringAssert.Contains(result.Xaml!, "Grid.RowSpan=\"2\"");
248+
CollectionAssert.Contains(result.OptionsResolved, "Item1RowSpan=2");
249+
Assert.AreEqual(0, result.OptionBindingsDropped.Count);
250+
}
251+
252+
[TestMethod]
253+
public void AnAttachedPropertyPathIsNotMistakenForACast()
254+
{
255+
// {Binding (Grid.Row)} is a path, not a cast: the parentheses are the whole path and
256+
// there is nothing after them. Stripping them would turn the path into the empty
257+
// string and lose the binding.
258+
var result = Extract("<Border Tag=\"{Binding (Grid.Row)}\" />");
259+
260+
Assert.IsNull(result.Error);
261+
StringAssert.Contains(result.Xaml!, "{Binding (Grid.Row)}");
262+
}
263+
264+
[TestMethod]
265+
public void MarkupInsideACommentIsNotRewritten()
266+
{
267+
// A comment ends at '-->', not at the first '>'. Stopping at the first one resumed the
268+
// scan inside the comment — here, straight after the <StackPanel> start tag — and
269+
// edited the author's commented-out alternative as though it were live markup.
270+
var result = Extract(
271+
"""
272+
<!--<StackPanel>
273+
<Button IsEnabled="{x:Bind IsCardEnabled}" />
274+
</StackPanel>-->
275+
<Border Background="Red" />
276+
""",
277+
new SampleOption("IsCardEnabled", "True"));
278+
279+
Assert.IsNull(result.Error);
280+
StringAssert.Contains(result.Xaml!, "<Button IsEnabled=\"{x:Bind IsCardEnabled}\" />");
281+
Assert.AreEqual(0, result.OptionsResolved.Count);
282+
Assert.AreEqual(0, result.OptionBindingsDropped.Count);
283+
}
284+
285+
[TestMethod]
286+
public void AnElementNameInsideACommentDoesNotCountAsDeclared()
287+
{
288+
// The commented-out element does not exist, so the binding that names it is dangling
289+
// and its attribute has to go. Reading the comment as markup would publish a binding
290+
// pointing at nothing.
291+
var result = Extract(
292+
"""
293+
<!--<StackPanel>
294+
<TextBox x:Name="Source" Text="Hi" />
295+
</StackPanel>-->
296+
<TextBlock Text="{Binding Text, ElementName=Source}" />
297+
""");
298+
299+
Assert.IsNull(result.Error);
300+
Assert.IsFalse(result.Xaml!.Contains("ElementName=Source", StringComparison.Ordinal));
301+
}
302+
303+
[TestMethod]
304+
public void APrefixUsedOnlyInsideACommentIsNotPublishedAsAnImport()
305+
{
306+
// An import tells the reader to reference a package. One needed only by markup the
307+
// author commented out is a package the published fragment does not use.
308+
var result = Extract(
309+
"""
310+
<!--<Border Background="{controls:SomeExtension}" />-->
311+
<Border Background="Red" />
312+
""");
313+
314+
Assert.AreEqual(0, result.XmlnsImports.Count);
315+
}
316+
317+
[TestMethod]
318+
public void AnApostropheInACommentDoesNotSwallowTheRestOfTheFragment()
319+
{
320+
// The prefix stripper tracks quotes so it does not stop at a '>' inside an attribute
321+
// value. Prose in a comment is not attribute values, and reading it as such opened a
322+
// quote that never closed, abandoning the rest of the fragment.
323+
var result = XamlFragment.Extract(
324+
"""
325+
<Page x:Class="Sample.MySample"
326+
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
327+
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
328+
xmlns:win="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
329+
<!-- The win: prefix doesn't survive extraction. -->
330+
<win:TextBox Text="Hi" />
331+
</Page>
332+
""",
333+
[]);
334+
335+
Assert.IsNull(result.Error);
336+
StringAssert.Contains(result.Xaml!, "<TextBox Text=\"Hi\" />");
337+
}
338+
236339
[TestMethod]
237340
public void UnsettledBindingsAcceptsMarkupWithNoBindings()
238341
{

‎tools/SampleIndexExporter/IndexGenerator.cs‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ private static List<string> GeneratedKeywords(MarkdownDocument document)
165165
List<WithheldSample> withheld,
166166
Dictionary<string, string> usedIds)
167167
{
168-
var id = UniqueId(document, component, issues, usedIds);
168+
var id = UniqueId(document, issues, usedIds);
169169
if (id is null)
170170
{
171171
return null;
@@ -361,16 +361,21 @@ private static Dictionary<string, SampleDeclaration> ReadDeclarations(
361361
}
362362

363363
/// <summary>
364-
/// Derive the entry's identifier from the documentation file name, and keep it unique.
364+
/// Derive the entry's identifier from the documentation file name.
365365
/// </summary>
366366
/// <remarks>
367367
/// The file name rather than the title, because consumers key sample ids off this and a
368-
/// title is prose that gets reworded. When two components document the same name, the
369-
/// component qualifies the second one rather than either silently winning.
368+
/// title is prose that gets reworded.
369+
///
370+
/// <para>Two components documenting the same file name is reported rather than resolved.
371+
/// Qualifying one of them with its component name would have to pick which one, and any
372+
/// rule for picking depends on the set of components present — so adding a component could
373+
/// change the id of an entry that already shipped, which is the one thing an identifier
374+
/// documented as stable must never do. A collision is a build failure a contributor settles
375+
/// by renaming, and every existing id stays where it is.</para>
370376
/// </remarks>
371377
private static string? UniqueId(
372378
MarkdownDocument document,
373-
string component,
374379
List<IndexIssue> issues,
375380
Dictionary<string, string> usedIds)
376381
{
@@ -382,17 +387,10 @@ private static Dictionary<string, SampleDeclaration> ReadDeclarations(
382387
return id;
383388
}
384389

385-
var qualified = $"{Slug(component)}-{id}";
386-
if (!usedIds.TryGetValue(qualified, out owner))
387-
{
388-
usedIds[qualified] = document.RelativePath;
389-
return qualified;
390-
}
391-
392390
issues.Add(new IndexIssue(
393391
IssueSeverity.Error,
394392
document.RelativePath,
395-
$"entry id '{qualified}' is already used by {owner}. Rename one of the documentation files."));
393+
$"entry id '{id}' is already used by {owner}. Rename one of the documentation files."));
396394

397395
return null;
398396
}

0 commit comments

Comments
 (0)