Skip to content

Fixes #5274 - Avoid duplicate ScreenChanged from Application.Screen setter#5404

Merged
tig merged 4 commits into
developfrom
copilot/fix-screen-setter-hang
May 25, 2026
Merged

Fixes #5274 - Avoid duplicate ScreenChanged from Application.Screen setter#5404
tig merged 4 commits into
developfrom
copilot/fix-screen-setter-hang

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 25, 2026

Proposed Changes/Todos

The fullscreen Application.Screen setter took a different path than Driver.SetScreenSize (...): it resized through the driver, then re-read Screen and raised ScreenChanged again. That extra work was enough to diverge from the known-safe test path used on CI.

  • Setter/event path

    • In fullscreen mode, Application.Screen now delegates to Driver.SetScreenSize (...) and relies on the existing Driver_SizeChanged path to raise ScreenChanged and invalidate layout exactly once.
    • Inline mode keeps its direct event raise, since there is no corresponding driver resize event.
  • Regression coverage

    • Added a focused test for pre-Begin (...) usage that verifies app.Screen = new Rectangle (...) raises ScreenChanged once and updates the effective screen size.
  • API guidance

    • Updated IApplication.Screen docs to call out that fullscreen Screen delegates to IDriver.SetScreenSize (...).
    • Documented that tests simulating terminal resize should prefer Driver.SetScreenSize (...), matching AppTestHelper.ResizeConsole (...).
using IApplication app = Application.Create ();
app.Init (DriverRegistry.Names.ANSI);

// Supported pre-Begin path
app.Screen = new Rectangle (0, 0, 100, 50);

// Canonical terminal-resize path for tests
app.Driver!.SetScreenSize (100, 50);

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

Copilot AI changed the title [WIP] Fix application.Screen setter hanging on headless Windows CI Fixes #issue. Avoid duplicate ScreenChanged from Application.Screen setter May 25, 2026
Copilot finished work on behalf of tig May 25, 2026 01:21
Copilot AI requested a review from tig May 25, 2026 01:21
@tig tig marked this pull request as ready for review May 25, 2026 03:30
@tig tig requested a review from Copilot May 25, 2026 03:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a duplicate ScreenChanged raise when setting IApplication.Screen in fullscreen mode by ensuring the setter follows the same path as IDriver.SetScreenSize(...), relying on the existing driver SizeChanged pipeline to invalidate layout and raise ScreenChanged exactly once.

Changes:

  • Updated ApplicationImpl.Screen setter to avoid explicitly raising ScreenChanged in fullscreen mode and instead rely on Driver_SizeChanged.
  • Added a regression test ensuring app.Screen = ... before Begin(...) raises ScreenChanged exactly once and updates app.Screen.
  • Updated IApplication.Screen XML docs to describe fullscreen behavior and recommended testing approach.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
Tests/UnitTestsParallelizable/Application/ScreenTests.cs Adds regression coverage for single ScreenChanged raise when setting Screen pre-Begin(...).
Terminal.Gui/App/IApplication.cs Documents that fullscreen Screen setter delegates to IDriver.SetScreenSize(...) and suggests using it in tests.
Terminal.Gui/App/ApplicationImpl.Screen.cs Removes the extra explicit ScreenChanged raise in fullscreen mode; relies on Driver_SizeChanged to raise/invalidate once.

Comment thread Terminal.Gui/App/IApplication.cs
@tig tig changed the title Fixes #issue. Avoid duplicate ScreenChanged from Application.Screen setter Fixes #5274 - Avoid duplicate ScreenChanged from Application.Screen setter May 25, 2026
Add a test verifying that ScreenChanged fires correctly when Screen is
set before Driver is initialized (Driver is null). Update the XML doc
remark on IApplication.Screen to accurately document the Driver != null
guard behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@tig tig requested a review from BDisp May 25, 2026 19:22
Copy link
Copy Markdown
Collaborator

@BDisp BDisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only to remember the XML warnings.

The cref referenced Init(string?,string?) but the actual signature is Init(string?).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Application.Screen setter hangs on headless Windows CI runners (ANSI driver)

4 participants