Skip to content
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

Add an environment variable to disable scale adjustment based on detected DPI #5359

Merged
merged 3 commits into from
Jan 6, 2025

Conversation

dweymouth
Copy link
Contributor

Description:

Adds a new environment variable FYNE_DISABLE_DPI_DETECTION which can be set to true, True, TRUE or 1 to disable detecting the screen DPI and adjusting scaling accordingly.

I do not have a dual-monitor Linux setup so I can only confirm that it results in a new Fyne app being drawn at 1:1 pixel scale on a Hi-DPI monitor.

Fixes #5164

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

@Jacalz
Copy link
Member

Jacalz commented Jan 4, 2025

I doubt many Linux users will set this environment variable. Is it not better to fix the issue and/or rework how we handle window scaling? Something is obviously wrong there from what I can tell from the ticket.

@dweymouth
Copy link
Contributor Author

dweymouth commented Jan 4, 2025

There's two separate things being discussed in the ticket. The original conversation was about something that's not a bug, but a user preference to disable a window re-adjusting its scale when moving between monitors. This PR is for that topic

@Jacalz
Copy link
Member

Jacalz commented Jan 4, 2025

Alright then but maybe it is better to add it into fyne_settings instead in that case? We could add a page for advanced options.

My gripe with this is that a user needs to read the documentation to know that the environment variable exists (that needs to be maintained and not rot away as well) and then define it either for each app individually or globally. I doubt many users will do so.

@dweymouth
Copy link
Contributor Author

Alright then but maybe it is better to add it into fyne_settings instead in that case? We could add a page for advanced options.

We could do that as well, similar to how FYNE_SCALE is done (I think). I'd like to avoid a Fyne app user having to download and run a different app (fyne settings) to adjust this.

@andydotxyz
Copy link
Member

I'd like to avoid a Fyne app user having to download and run a different app (fyne settings) to adjust this.

You can embed the Fyne settings panel in your app if you like :)

@dweymouth
Copy link
Contributor Author

dweymouth commented Jan 4, 2025

I'd like to avoid a Fyne app user having to download and run a different app (fyne settings) to adjust this.

You can embed the Fyne settings panel in your app if you like :)

Won't that adjust it for all apps though instead of just mine? Which is not what the user would expect. And also I already have my own settings UI with other settings specific to my app.

I can add this to the fyne_settings app as well though, keeping in mind it would need a new public getter API like fyne.CurrentApp().Settings().DPIDetectionDisabled(). But I strongly would urge keeping the env variable too, like we have for FYNE_SCALE

What do you think?

@andydotxyz
Copy link
Member

Won't that adjust it for all apps though instead of just mine?

Yes

@Jacalz
Copy link
Member

Jacalz commented Jan 4, 2025

Won't that adjust it for all apps though instead of just mine? Which is not what the user would expect.

If the app is sandboxed, like Flatpak for example, then it is only for your application.

@dweymouth
Copy link
Contributor Author

If the app is sandboxed, like Flatpak for example, then it is only for your application.

Yes, though not all installations will be Flatpak. It sounds like you're in favor of adding a settings app control for this as well as the env var?

// check if DPI detection is disabled
env := os.Getenv(disableDPIDetectionEnvKey)
switch env {
case "True", "TRUE", "true", "T", "t", "1":
Copy link
Member

Choose a reason for hiding this comment

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

You might want to use strings.EqualFold instead of hardcoding values. That should allow the word "true" no matter how it is capitalised.

case "True", "TRUE", "true", "T", "t", "1":
return 1
}

if build.IsWayland { // Wayland controls scale through content scaling
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be wise to move the env check to after this check. In the case of Wayland, there is no need to check the environment variable at all and the compiler can just deadcode eliminate most of the function.

@Jacalz
Copy link
Member

Jacalz commented Jan 4, 2025

Yes, though not all installations will be Flatpak.

Indeed. I just wanted to add that information to the discussion :)

It sounds like you're in favor of adding a settings app control for this as well as the env var?

Was this meant as a setting mostly to help out on Linux or is it useful on all platforms? I was initially in favour of the settings page but in my latest review I looked at the code and saw that Wayland handles scaling themselves. If it is Linux specific, it seems strange to add a settings toggle for it when Wayland will take over eventually (not talking about the ENV var).

@dweymouth
Copy link
Contributor Author

dweymouth commented Jan 4, 2025

I do think it is mainly a Linux X11 thing, since at least MacOS handles it at the OS level I think (not really sure about Windows). So yeah maybe just env var makes sense.

@andydotxyz
Copy link
Member

I do think it is mainly a Linux X11 thing, since at least MacOS handles it at the OS level I think (not really sure about Windows). So yeah maybe just env var makes sense.

macOS and Windows provide a system check first - so global user settings are respected.

@dweymouth
Copy link
Contributor Author

Are there any other code suggestions for this?

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Cool, thanks for this, I'm sure a few folk will like to control scaling at a lower level.

@dweymouth dweymouth merged commit 1634c22 into fyne-io:develop Jan 6, 2025
12 checks passed
@dweymouth dweymouth deleted the disable-dpi-detection branch January 6, 2025 23:25
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.

3 participants