-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Ensure Canvas Matches Actual Window Size #5797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
cwarden
wants to merge
2
commits into
fyne-io:develop
Choose a base branch
from
cwarden:scaling-fix
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| //go:build !wasm && !test_web_driver && !mobile && !no_glfw | ||
|
|
||
| package glfw | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "fyne.io/fyne/v2" | ||
| "fyne.io/fyne/v2/container" | ||
| "fyne.io/fyne/v2/widget" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestWindow_RescaleContext_Basic(t *testing.T) { | ||
| // Basic test to ensure RescaleContext doesn't panic | ||
| w := createWindow("Test") | ||
| defer w.Close() | ||
| w.SetContent(widget.NewLabel("Test")) | ||
|
|
||
| // Test that RescaleContext can be called without panic | ||
| runOnMain(func() { | ||
| w.RescaleContext() | ||
| }) | ||
|
|
||
| // Verify window still has valid canvas | ||
| assert.NotNil(t, w.Canvas()) | ||
| assert.NotNil(t, w.Canvas().Content()) | ||
| } | ||
|
|
||
| func TestWindow_RescaleContext_WithContent(t *testing.T) { | ||
| // Test RescaleContext with actual content | ||
| w := createWindow("Test") | ||
| defer w.Close() | ||
|
|
||
| content := container.NewVBox( | ||
| widget.NewLabel("Test App"), | ||
| widget.NewButton("Click Me", func() {}), | ||
| ) | ||
| w.SetContent(content) | ||
|
|
||
| // Call RescaleContext | ||
| runOnMain(func() { | ||
| w.RescaleContext() | ||
| }) | ||
|
|
||
| // Verify content is preserved | ||
| assert.Equal(t, content, w.Canvas().Content()) | ||
| assert.Greater(t, w.Canvas().Size().Width, float32(0)) | ||
| assert.Greater(t, w.Canvas().Size().Height, float32(0)) | ||
| } | ||
|
|
||
| func TestWindow_RescaleContext_MinSize(t *testing.T) { | ||
| // Test that RescaleContext respects minimum size | ||
| w := createWindow("Test") | ||
| defer w.Close() | ||
|
|
||
| label := widget.NewLabel("Test with minimum size") | ||
| label.Resize(fyne.NewSize(300, 200)) | ||
| w.SetContent(label) | ||
|
|
||
| // Call RescaleContext | ||
| runOnMain(func() { | ||
| w.RescaleContext() | ||
| }) | ||
|
|
||
| // Canvas should respect content minimum size | ||
| minSize := label.MinSize() | ||
| canvasSize := w.Canvas().Size() | ||
| assert.GreaterOrEqual(t, canvasSize.Width, minSize.Width) | ||
| assert.GreaterOrEqual(t, canvasSize.Height, minSize.Height) | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the source of the change in behavior - with previous setup it was correct to adjust the window size based on the content and scale so that when moving when scale changes the window changed too (worked on all except gnome moving from a low scale to high scale monitor - desktop would not allow the window to shrink).
I wonder if there is a way to fix tiling whilst maintaining the old behavior?
I don't really know why tiling breaks in this code so I'm not sure if there is a happy medium.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the issue that fyne can't control the window size in every environment? But it can control the canvas size so when it doesn't get the window size it wants, it needs to adjust the canvas size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense.
So instead of trying to control it with this PR it doesn't try any more? Can we somehow attempt to resize and fallback if it doesn't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't regularly work with non-tiled windows, but the automatic resizing feels awkward to me.
The behavior with the changes in this branch feel more intuitive to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would want to test this PR again but I do agree that using this PR felt more natural when I moved the window between screens.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to compare how other apps behave when moving them between windows. This is a GTK4 app:
Screencast.From.2025-09-06.08-20-01.webm
This is Fyne before this change (jumps a bit in size also):
Screencast.From.2025-09-06.08-20-38.webm
This is with this PR:
Screencast.From.2025-09-06.08-23-05.webm
With this, it actually behaves more like the other apps on my system and like what I would expect. The scaling and/or window size seems to potentially be a bit smaller than before though, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that comparing to GTK+ is a good idea - they have declared that fractional scaling on X11 is not possible and rely on gnome desktop hinting instead of DPI detection.
Not apples and Oranges.
If you compare with how apps work on Windows or macOS you will see the scale changes and the size changes so that the UI is a consistent size unless user specifies other default for the monitors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want the GTK version then turn off DPI detection completely rather than removing it from the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does their opinion on X11 and fractional scaling have to do with it? I am using Wayland and have 125% fractional scaling enabled on the right monitor. It works perfectly. The behaviour of this app seems more sane after this change than before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry - are you doing all of this testing with pure-wayland compiled Fyne? I did not realise that was the case. If so it is a different code path and I will have to re-analyse as the wayland scale handling is not the same as X11.