Skip to content

Commit 549436e

Browse files
Copilottig
Copilot
and
tig
authored
Fixes #4177: View.GetAttributeForRole now defers to SuperView for proper attribute hierarchy (#4292)
* Initial plan * Fix GetAttributeForRole to defer to SuperView when no explicit scheme Co-authored-by: tig <585482+tig@users.noreply.github.com> * Add test for Adornment attribute resolution Co-authored-by: tig <585482+tig@users.noreply.github.com> * Fix: Also check SchemeName when deferring to SuperView in GetAttributeForRole Co-authored-by: tig <585482+tig@users.noreply.github.com> * Add test for StatusBar/Bar not deferring when SchemeName is set Co-authored-by: tig <585482+tig@users.noreply.github.com> * Add comprehensive low-level tests for GetAttributeForRole hierarchy Co-authored-by: tig <585482+tig@users.noreply.github.com> * Update AGENTS.md with PR branch pull instructions Co-authored-by: tig <585482+tig@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tig <585482+tig@users.noreply.github.com> Co-authored-by: Tig <tig@users.noreply.github.com>
1 parent 86b7996 commit 549436e

File tree

4 files changed

+359
-1
lines changed

4 files changed

+359
-1
lines changed

‎AGENTS.md‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,3 +130,50 @@ dotnet test
130130
1. Maintain existing code structure and organization unless explicitly told
131131
2. View sub-classes must not use private APIs
132132
3. Suggest changes to the `./docfx/docs/` folder when appropriate
133+
134+
## Working with Pull Request Branches
135+
136+
When creating PRs, include instructions at the end of each PR description for how to pull the branch down locally. Use the following template, adapted for the typical remote setup where `origin` points to the user's fork and `upstream` points to `gui-cs/Terminal.Gui`:
137+
138+
```markdown
139+
## How to Pull This PR Branch Locally
140+
141+
If you want to test or modify this PR locally, use one of these approaches based on your remote setup:
142+
143+
### Method 1: Fetch from upstream (if branch exists there)
144+
```bash
145+
# Fetch the branch from upstream
146+
git fetch upstream <branch-name>
147+
148+
# Switch to the branch
149+
git checkout <branch-name>
150+
151+
# Make your changes, then commit them
152+
git add .
153+
git commit -m "Your commit message"
154+
155+
# Push to your fork (origin)
156+
git push origin <branch-name>
157+
```
158+
159+
### Method 2: Fetch by PR number
160+
```bash
161+
# Fetch the PR branch from upstream by PR number
162+
git fetch upstream pull/<PR_NUMBER>/head:<branch-name>
163+
164+
# Switch to the branch
165+
git checkout <branch-name>
166+
167+
# Make your changes, then commit them
168+
git add .
169+
git commit -m "Your commit message"
170+
171+
# Push to your fork (origin)
172+
git push origin <branch-name>
173+
```
174+
175+
The PR will automatically update when you push to the branch in your fork.
176+
```
177+
178+
**Note:** Adjust the remote names if your setup differs (e.g., if `origin` points to `gui-cs/Terminal.Gui` and you have a separate remote for your fork).
179+

‎Terminal.Gui/ViewBase/View.Drawing.Attribute.cs‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,20 @@ public partial class View
3535
/// <returns>The corresponding <see cref="Attribute"/> from the <see cref="Drawing.Scheme"/>.</returns>
3636
public Attribute GetAttributeForRole (VisualRole role)
3737
{
38-
Attribute schemeAttribute = GetScheme ()!.GetAttributeForRole (role);
38+
// Get the base attribute
39+
// If this view doesn't have an explicit scheme or scheme name, defer to SuperView for attribute resolution.
40+
// This allows parent views to customize attribute resolution for their children via
41+
// OnGettingAttributeForRole/GettingAttributeForRole.
42+
// This matches the logic in GetScheme() where SchemeName takes precedence over SuperView inheritance.
43+
Attribute schemeAttribute;
44+
if (!HasScheme && string.IsNullOrEmpty (SchemeName) && SuperView is { })
45+
{
46+
schemeAttribute = SuperView.GetAttributeForRole (role);
47+
}
48+
else
49+
{
50+
schemeAttribute = GetScheme ()!.GetAttributeForRole (role);
51+
}
3952

4053
if (OnGettingAttributeForRole (role, ref schemeAttribute))
4154
{

‎Tests/UnitTestsParallelizable/View/SchemeTests.cs‎

Lines changed: 265 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,4 +269,269 @@ public void View_Resolves_Attributes_From_Scheme ()
269269
view.Dispose ();
270270
}
271271

272+
[Fact]
273+
public void GetAttributeForRole_SubView_DefersToSuperView_WhenNoExplicitScheme ()
274+
{
275+
var parentView = new View { SchemeName = "Base" };
276+
var childView = new View ();
277+
parentView.Add (childView);
278+
279+
// Parent customizes attribute resolution
280+
var customAttribute = new Attribute (Color.BrightMagenta, Color.BrightGreen);
281+
parentView.GettingAttributeForRole += (sender, args) =>
282+
{
283+
if (args.Role == VisualRole.Normal)
284+
{
285+
args.Result = customAttribute;
286+
args.Handled = true;
287+
}
288+
};
289+
290+
// Child without explicit scheme should get customized attribute from parent
291+
Assert.Equal (customAttribute, childView.GetAttributeForRole (VisualRole.Normal));
292+
293+
childView.Dispose ();
294+
parentView.Dispose ();
295+
}
296+
297+
[Fact]
298+
public void GetAttributeForRole_SubView_UsesOwnScheme_WhenExplicitlySet ()
299+
{
300+
var parentView = new View { SchemeName = "Base" };
301+
var childView = new View ();
302+
parentView.Add (childView);
303+
304+
// Set explicit scheme on child
305+
var childScheme = SchemeManager.GetHardCodedSchemes ()? ["Dialog"];
306+
childView.SetScheme (childScheme);
307+
308+
// Parent customizes attribute resolution
309+
var customAttribute = new Attribute (Color.BrightMagenta, Color.BrightGreen);
310+
parentView.GettingAttributeForRole += (sender, args) =>
311+
{
312+
if (args.Role == VisualRole.Normal)
313+
{
314+
args.Result = customAttribute;
315+
args.Handled = true;
316+
}
317+
};
318+
319+
// Child with explicit scheme should NOT get customized attribute from parent
320+
Assert.NotEqual (customAttribute, childView.GetAttributeForRole (VisualRole.Normal));
321+
Assert.Equal (childScheme!.Normal, childView.GetAttributeForRole (VisualRole.Normal));
322+
323+
childView.Dispose ();
324+
parentView.Dispose ();
325+
}
326+
327+
[Fact]
328+
public void GetAttributeForRole_Adornment_UsesParentScheme ()
329+
{
330+
// Border (an Adornment) doesn't have a SuperView but should use its Parent's scheme
331+
var view = new View { SchemeName = "Dialog" };
332+
var border = view.Border!;
333+
334+
Assert.NotNull (border);
335+
Assert.Null (border.SuperView); // Adornments don't have SuperView
336+
Assert.NotNull (border.Parent);
337+
338+
var dialogScheme = SchemeManager.GetHardCodedSchemes ()? ["Dialog"];
339+
340+
// Border should use its Parent's scheme, not Base
341+
Assert.Equal (dialogScheme!.Normal, border.GetAttributeForRole (VisualRole.Normal));
342+
343+
view.Dispose ();
344+
}
345+
346+
[Fact]
347+
public void GetAttributeForRole_SubView_UsesSchemeName_WhenSet ()
348+
{
349+
var parentView = new View { SchemeName = "Base" };
350+
var childView = new View ();
351+
parentView.Add (childView);
352+
353+
// Set SchemeName on child (not explicit scheme)
354+
childView.SchemeName = "Dialog";
355+
356+
// Parent customizes attribute resolution
357+
var customAttribute = new Attribute (Color.BrightMagenta, Color.BrightGreen);
358+
parentView.GettingAttributeForRole += (sender, args) =>
359+
{
360+
if (args.Role == VisualRole.Normal)
361+
{
362+
args.Result = customAttribute;
363+
args.Handled = true;
364+
}
365+
};
366+
367+
// Child with SchemeName should NOT get customized attribute from parent
368+
// It should use the Dialog scheme instead
369+
var dialogScheme = SchemeManager.GetHardCodedSchemes ()? ["Dialog"];
370+
Assert.NotEqual (customAttribute, childView.GetAttributeForRole (VisualRole.Normal));
371+
Assert.Equal (dialogScheme!.Normal, childView.GetAttributeForRole (VisualRole.Normal));
372+
373+
childView.Dispose ();
374+
parentView.Dispose ();
375+
}
376+
377+
[Fact]
378+
public void GetAttributeForRole_NestedHierarchy_DefersCorrectly ()
379+
{
380+
// Test: grandchild without explicit scheme defers through parent to grandparent
381+
// Would fail without the SuperView deferral fix (commit 154ac15)
382+
383+
var grandparentView = new View { SchemeName = "Base" };
384+
var parentView = new View (); // No scheme or SchemeName
385+
var childView = new View (); // No scheme or SchemeName
386+
387+
grandparentView.Add (parentView);
388+
parentView.Add (childView);
389+
390+
// Grandparent customizes attributes
391+
var customAttribute = new Attribute (Color.BrightYellow, Color.BrightBlue);
392+
grandparentView.GettingAttributeForRole += (sender, args) =>
393+
{
394+
if (args.Role == VisualRole.Normal)
395+
{
396+
args.Result = customAttribute;
397+
args.Handled = true;
398+
}
399+
};
400+
401+
// Child should get attribute from grandparent through parent
402+
Assert.Equal (customAttribute, childView.GetAttributeForRole (VisualRole.Normal));
403+
404+
// Parent should also get attribute from grandparent
405+
Assert.Equal (customAttribute, parentView.GetAttributeForRole (VisualRole.Normal));
406+
407+
childView.Dispose ();
408+
parentView.Dispose ();
409+
grandparentView.Dispose ();
410+
}
411+
412+
[Fact]
413+
public void GetAttributeForRole_ParentWithSchemeNameBreaksChain ()
414+
{
415+
// Test: parent with SchemeName stops deferral chain
416+
// Would fail without the SchemeName check (commit 866e002)
417+
418+
var grandparentView = new View { SchemeName = "Base" };
419+
var parentView = new View { SchemeName = "Dialog" }; // Sets SchemeName
420+
var childView = new View (); // No scheme or SchemeName
421+
422+
grandparentView.Add (parentView);
423+
parentView.Add (childView);
424+
425+
// Grandparent customizes attributes
426+
var customAttribute = new Attribute (Color.BrightYellow, Color.BrightBlue);
427+
grandparentView.GettingAttributeForRole += (sender, args) =>
428+
{
429+
if (args.Role == VisualRole.Normal)
430+
{
431+
args.Result = customAttribute;
432+
args.Handled = true;
433+
}
434+
};
435+
436+
// Parent should NOT get grandparent's customization (it has SchemeName)
437+
var dialogScheme = SchemeManager.GetHardCodedSchemes ()? ["Dialog"];
438+
Assert.NotEqual (customAttribute, parentView.GetAttributeForRole (VisualRole.Normal));
439+
Assert.Equal (dialogScheme!.Normal, parentView.GetAttributeForRole (VisualRole.Normal));
440+
441+
// Child should get parent's Dialog scheme (defers to parent, parent uses Dialog scheme)
442+
Assert.Equal (dialogScheme!.Normal, childView.GetAttributeForRole (VisualRole.Normal));
443+
444+
childView.Dispose ();
445+
parentView.Dispose ();
446+
grandparentView.Dispose ();
447+
}
448+
449+
[Fact]
450+
public void GetAttributeForRole_OnGettingAttributeForRole_TakesPrecedence ()
451+
{
452+
// Test: view's own OnGettingAttributeForRole takes precedence over parent
453+
// This should work with or without the fix, but validates precedence
454+
455+
var parentView = new View { SchemeName = "Base" };
456+
var childView = new TestViewWithAttributeOverride ();
457+
parentView.Add (childView);
458+
459+
// Parent customizes attributes
460+
var parentAttribute = new Attribute (Color.BrightYellow, Color.BrightBlue);
461+
parentView.GettingAttributeForRole += (sender, args) =>
462+
{
463+
if (args.Role == VisualRole.Normal)
464+
{
465+
args.Result = parentAttribute;
466+
args.Handled = true;
467+
}
468+
};
469+
470+
// Child's own override should take precedence
471+
var childOverrideAttribute = new Attribute (Color.BrightRed, Color.BrightCyan);
472+
childView.OverrideAttribute = childOverrideAttribute;
473+
474+
Assert.Equal (childOverrideAttribute, childView.GetAttributeForRole (VisualRole.Normal));
475+
476+
childView.Dispose ();
477+
parentView.Dispose ();
478+
}
479+
480+
[Fact]
481+
public void GetAttributeForRole_MultipleRoles_DeferCorrectly ()
482+
{
483+
// Test: multiple VisualRoles all defer correctly
484+
// Would fail without the SuperView deferral fix for any role
485+
486+
var parentView = new View { SchemeName = "Base" };
487+
var childView = new View ();
488+
parentView.Add (childView);
489+
490+
var normalAttr = new Attribute (Color.Red, Color.Blue);
491+
var focusAttr = new Attribute (Color.Green, Color.Yellow);
492+
var hotNormalAttr = new Attribute (Color.Magenta, Color.Cyan);
493+
494+
parentView.GettingAttributeForRole += (sender, args) =>
495+
{
496+
switch (args.Role)
497+
{
498+
case VisualRole.Normal:
499+
args.Result = normalAttr;
500+
args.Handled = true;
501+
break;
502+
case VisualRole.Focus:
503+
args.Result = focusAttr;
504+
args.Handled = true;
505+
break;
506+
case VisualRole.HotNormal:
507+
args.Result = hotNormalAttr;
508+
args.Handled = true;
509+
break;
510+
}
511+
};
512+
513+
// All roles should defer to parent
514+
Assert.Equal (normalAttr, childView.GetAttributeForRole (VisualRole.Normal));
515+
Assert.Equal (focusAttr, childView.GetAttributeForRole (VisualRole.Focus));
516+
Assert.Equal (hotNormalAttr, childView.GetAttributeForRole (VisualRole.HotNormal));
517+
518+
childView.Dispose ();
519+
parentView.Dispose ();
520+
}
521+
522+
private class TestViewWithAttributeOverride : View
523+
{
524+
public Attribute? OverrideAttribute { get; set; }
525+
526+
protected override bool OnGettingAttributeForRole (in VisualRole role, ref Attribute currentAttribute)
527+
{
528+
if (OverrideAttribute.HasValue && role == VisualRole.Normal)
529+
{
530+
currentAttribute = OverrideAttribute.Value;
531+
return true;
532+
}
533+
return base.OnGettingAttributeForRole (role, ref currentAttribute);
534+
}
535+
}
536+
272537
}

‎Tests/UnitTestsParallelizable/Views/BarTests.cs‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,4 +102,37 @@ public void Layout_ChangesBasedOnOrientation ()
102102
bar.LayoutSubViews ();
103103
// TODO: Assert specific layout expectations for vertical orientation
104104
}
105+
106+
[Fact]
107+
public void GetAttributeForRole_DoesNotDeferToSuperView_WhenSchemeNameIsSet ()
108+
{
109+
// This test would fail before the fix that checks SchemeName in GetAttributeForRole
110+
// StatusBar and MenuBarv2 set SchemeName = "Menu", and should use Menu scheme
111+
// instead of deferring to parent's customized attributes
112+
113+
var parentView = new View { SchemeName = "Base" };
114+
var statusBar = new StatusBar ();
115+
parentView.Add (statusBar);
116+
117+
// Parent customizes attribute resolution
118+
var customAttribute = new Attribute (Color.BrightMagenta, Color.BrightGreen);
119+
parentView.GettingAttributeForRole += (sender, args) =>
120+
{
121+
if (args.Role == VisualRole.Normal)
122+
{
123+
args.Result = customAttribute;
124+
args.Handled = true;
125+
}
126+
};
127+
128+
// StatusBar sets SchemeName = "Menu" in its constructor
129+
// Before the fix: StatusBar would defer to parent and get customAttribute (WRONG)
130+
// After the fix: StatusBar uses Menu scheme (CORRECT)
131+
var menuScheme = SchemeManager.GetHardCodedSchemes ()? ["Menu"];
132+
Assert.NotEqual (customAttribute, statusBar.GetAttributeForRole (VisualRole.Normal));
133+
Assert.Equal (menuScheme!.Normal, statusBar.GetAttributeForRole (VisualRole.Normal));
134+
135+
statusBar.Dispose ();
136+
parentView.Dispose ();
137+
}
105138
}

0 commit comments

Comments
 (0)