Skip to content

Conversation

@softworkz
Copy link
Collaborator

Electron's default is trueElectron's default for FullScreenable is true

Copilot AI review requested due to automatic review settings November 20, 2025 01:59
Copilot finished reviewing on behalf of softworkz November 20, 2025 02:00
Copy link

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 the default value for the Fullscreenable property in BrowserWindowOptions to match Electron's true default and adds comprehensive integration tests for window state management. The PR title contains a minor formatting issue ("BrowserWindowOptions; Fix default and add more Tests" has an extra space before "and").

  • Fixed Fullscreenable property to default to true instead of false, aligning with Electron's behavior
  • Added resource cleanup (try-finally blocks) to existing tests to prevent resource leaks
  • Added six new integration tests covering fullscreen, maximize, minimize operations and their associated events

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/ElectronNET.API/API/Entities/BrowserWindowOptions.cs Fixed Fullscreenable default value to true with [DefaultValue(true)] attribute and inline initialization, updated documentation comment
src/ElectronNET.IntegrationTests/Tests/BrowserWindowTests.cs Wrapped existing tests in try-finally blocks for proper cleanup, added 6 new tests for window state operations, and introduced a WaitUntil helper method for polling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@softworkz softworkz force-pushed the submit_outpath branch 6 times, most recently from bb2eb4e to 1815951 Compare November 20, 2025 03:50
Copy link
Collaborator

@FlorianRappl FlorianRappl left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@FlorianRappl FlorianRappl merged commit 1d554fd into ElectronNET:develop Nov 20, 2025
9 of 10 checks passed
@FlorianRappl FlorianRappl added this to the 0.3.0 milestone Nov 20, 2025
@softworkz
Copy link
Collaborator Author

This probably the explanation for why that serialization option "IgnoreDefaults" has been there.

I'm not sure whether this is already the right thing. Probably there are types of windows (like tool window or dialog) which may not be "FullScreenable" and Electron might error.
That would be my guess due to the fact that all other properties have defauilt values defined - but not this one.

I think the most safe and correct way would be to make all non-string properties nullable. This comes closest to the JS development behavior where you just omit those properties that you don't want to set.

Right now we need to see which defaults Electron has and set them accordingly. But when there's no constant default, this doesn't work out. Most likely, that's where somebody came up with the "IgnoreDefault" serialization option...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants