scarab: Apply scaling factor in Wayland #348427

l0b0 · 2024-10-14T04:24:48Z

Sets AVALONIA_GLOBAL_SCALE_FACTOR to the GNOME desktop scaling factor based on
AvaloniaUI/Avalonia#9390 (comment).

Also:

Removed seemingly unnecessary dependencies
Formatted using shfmt --write pkgs/tools/games/scarab/scaling-settings.bash (necessitated copying the relevant .editorconfig settings)
Linted using shellcheck --enable=all --severity=style pkgs/tools/games/scarab/scaling-settings.bash

Thanks to @Artturin for help on Matrix!

Things done

Add a 👍 reaction to pull requests you find important.

huantianad

Looks pretty good, I am curious though, since this is an avalonia issue, anyway we can apply this for all avalonia apps?

pkgs/tools/games/scarab/default.nix

l0b0 · 2024-10-14T08:12:30Z

Looks pretty good, I am curious though, since this is an avalonia issue, anyway we can apply this for all avalonia apps?

Good question. I'm not at all familiar with Avalonia, and a quick search for it in nixpkgs didn't reveal anything obvious. @corngood any ideas?

corngood · 2024-10-14T14:03:47Z

pkgs/build-support/dotnet/fetch-nupkg/overrides.nix has:

  "Avalonia.X11" =
    package:
    package.overrideAttrs (
      old:
      lib.optionalAttrs (!stdenv.hostPlatform.isDarwin) {
        setupHook = writeText "setupHook.sh" ''
          prependToVar dotnetRuntimeDeps \
            "${lib.getLib libICE}" \
            "${lib.getLib libSM}" \
            "${lib.getLib libX11}"
        '';
      }
    );

This is probably where you'd want to do it. You probably couldn't use dotnetRuntimeDeps, but you might be able to use makeWrapperArgs. I think something like --run "source ""${./scaling-settings.bash}" might allow you to modify the environment in the wrapper, but I haven't tried it.

huantianad · 2024-11-05T19:38:32Z

Wanted to bump this again, do we want to apply this change just for scarab for now, and for all avalonia apps later?

corngood · 2024-11-05T20:32:42Z

From the upstream issue:

Avalonia currently reads Xft.dpi value from XRDB, which is the only thing we can do since WM/DE authors simply refuse to provide accurate per-monitor scaling information for X11 apps.

It seems like we're reproducing this logic. I don't understand why it's not working out of the box.

l0b0 · 2024-11-05T22:16:39Z

It seems like we're reproducing this logic. I don't understand why it's not working out of the box.

Same, but this works right now. I'd rather just revert once a more suitable solution is found. I don't know the first thing about Avalonia or how Scarab is built, and I don't have time to work out a better solution.

corngood · 2024-11-05T20:09:24Z

pkgs/tools/games/scarab/scaling-settings.bash

+# Keep existing value if it is already non-empty
+if [[ -z "${AVALONIA_GLOBAL_SCALE_FACTOR-}" ]]; then
+    # Attempt to get GNOME desktop interface scaling factor
+    AVALONIA_GLOBAL_SCALE_FACTOR="$(gsettings get org.gnome.desktop.interface scaling-factor | cut --delimiter=' ' --fields=2)"


If I run gsettings get org.gnome.desktop.interface scaling-factor | cut --delimiter=' ' --fields=2 on my system I get 0. Would this be a problem?

For the setting I get uint32 0, using xmonad on xorg.

gsettings describe says

Integer factor used to scale windows by. For use on high-dpi screens. 0 means pick automatically based on monitor.

So I guess falling through to the other methods would make sense.

Fixed by assuming that a zero from gsettings means I'd also get the same or another meaningless value from xrdb, and simply unsetting the scaling factor if that happens.

corngood · 2024-11-05T20:10:24Z

pkgs/tools/games/scarab/scaling-settings.bash

+# Keep existing value if it is already non-empty
+if [[ -z "${AVALONIA_GLOBAL_SCALE_FACTOR-}" ]]; then
+    # Attempt to get GNOME desktop interface scaling factor
+    AVALONIA_GLOBAL_SCALE_FACTOR="$(gsettings get org.gnome.desktop.interface scaling-factor | cut --delimiter=' ' --fields=2)"


Should we check for existence of gsettings instead of bringing it as a dependency?

Since it doesn't make sense to run Scarab in a pure shell, and since non-GNOME users probably won't have gsettings, I think you're right that we shouldn't pull it in. Done.

pkgs/tools/games/scarab/scaling-settings.bash

huantianad · 2024-11-05T22:47:24Z

From the upstream issue:

Avalonia currently reads Xft.dpi value from XRDB, which is the only thing we can do since WM/DE authors simply refuse to provide accurate per-monitor scaling information for X11 apps.

It seems like we're reproducing this logic. I don't understand why it's not working out of the box.

The issue is really strange, it does sound like this should be things that upstream should be and already is doing. Perhaps it has to do with XWayland, and how different DEs handle scaling with X11 apps running in XWayland.

l0b0 · 2024-11-06T00:02:43Z

All done @corngood, thank you!

corngood · 2024-11-06T00:19:06Z

pkgs/tools/games/scarab/default.nix

  buildDotnetModule,
  fetchFromGitHub,
-  glibc,


Removing these is good if they aren't needed, but I think it should be a separate commit from the wrapper, in case we need to bisect/revert.

corngood · 2024-11-06T00:25:42Z

pkgs/tools/games/scarab/scaling-settings.bash

@@ -0,0 +1,24 @@
+# Keep existing value if it is already non-empty
+if [[ -z "${AVALONIA_GLOBAL_SCALE_FACTOR-}" ]] && type gsettings >/dev/null; then
+    echo 'Attempting to get GNOME desktop interface scaling factor'


I would probably have made these echos go to stderr, since this is a wrapper.

corngood

This all looks good to me. Thanks!

The only question I have is: are there any automated editorconfig checks that could possibly fail because of the added coverage of .bash files?

I'll try to answer that myself, but if maybe someone knows for sure.

l0b0 · 2024-11-10T00:56:05Z

The only question I have is: are there any automated editorconfig checks that could possibly fail because of the added coverage of .bash files?

Oh, good point! editorconfig-checker $(find . -name '*.bash') does find a handful of wrong indentations, that's all. I've fixed those in a separate PR, and removed the EditorConfig commit from this PR.

corngood · 2024-11-10T00:58:52Z

are there any automated editorconfig checks that could possibly fail because of the added coverage of .bash files?

... There are editorconfig checks. They are usually only run on files changed in the PR. There are quite a few existing errors in master when it's run on the whole repo, but I verified that there are no errors in .bash files on this PR.

corngood · 2024-11-10T01:08:41Z

I'll fix those in a separate PR. I've removed that commit.

@l0b0 I ran the same checks, but with -disable-indent-size, because that's what seems to be used in the workflow:

https://github.com/NixOS/nixpkgs/actions/runs/11760835386/workflow?pr=348427

So if it's not going to fail in the regular checks, I think it's fine if you don't fix the other files. It wouldn't hurt though. Up to you...

l0b0 · 2024-11-10T01:11:06Z

I'll fix those in a separate PR. I've removed that commit.

@l0b0 I ran the same checks, but with -disable-indent-size, because that's what seems to be used in the workflow:

NixOS/nixpkgs/actions/runs/11760835386/workflow?pr=348427

So if it's not going to fail in the regular checks, I think it's fine if you don't fix the other files. It wouldn't hurt though. Up to you...

OIC, thanks! I didn't look at the indent size check. I'll leave it in for now, and see what people say. I assume the indent check is only disabled temporarily, until more files pass, but worst case that just means we'll commit the .editorconfig change itself. I'll split it into two commits to make this even easier.

corngood · 2024-11-10T01:48:04Z

Sorry, I should have actually tried to run this before. Without gsettings available I get this log noise:

/nix/store/z3fbayxg8aj5acjpmyfk6ln2cws8fd0l-scaling-settings.bash: line 2: type: gsettings: not found

I think using command -v instead of type would fix this. Or maybe type -p? I'm not 100% sure of the pros/cons of those. Looks like the former is a bit more common in the repo.

If $DISPLAY is not set, I xrdb fails and doesn't try to start Scarab. That's presumably the same with any failure of either xrdb or gsettings. In the case of DISPLAY not being set, the app won't even run, so maybe it's not a big deal. However, if we were going to apply this script to more packages, we'd have to be careful. I believe some of them have non-graphical interfaces using the same executable (even if it's just something like --version handling).

Also...

On my laptop I'm using gnome+wayland, and 192 dpi. For some reason, gsettings returns uint32 0, and xrdb reports 192. Because the check for 0 is after the xrdb block, it never actually tries to run xrdb, and launches without a scaling setting. Perhaps the 0 check should be specific to gsettings, since it's a documented default there.

Sets `AVALONIA_GLOBAL_SCALE_FACTOR` to the GNOME desktop scaling factor based on <AvaloniaUI/Avalonia#9390 (comment)>, falling back to the X FreeType DPI setting if the former is not available. Does not include `gsettings` and `xrdb` in build inputs, since these should be available on the relevant platforms. Bash does not support decimal/floating point arithmetic, so this *does* include `bc` in the runtime dependencies.

l0b0 · 2024-11-10T02:03:35Z

Sorry, I should have actually tried to run this before. Without gsettings available I get this log noise:
/nix/store/z3fbayxg8aj5acjpmyfk6ln2cws8fd0l-scaling-settings.bash: line 2: type: gsettings: not found
I think using command -v instead of type would fix this. Or maybe type -p? I'm not 100% sure of the pros/cons of those. Looks like the former is a bit more common in the repo.

Yes, command -v is usually recommended over type. I originally used type in the (admittedly extremely unlikely) case that someone was using a function or alias to point to a gsettings not on $PATH, but log noise is probably a good enough reason to use command -v instead.

If $DISPLAY is not set, I xrdb fails and doesn't try to start Scarab. That's presumably the same with any failure of either xrdb or gsettings. In the case of DISPLAY not being set, the app won't even run, so maybe it's not a big deal. However, if we were going to apply this script to more packages, we'd have to be careful. I believe some of them have non-graphical interfaces using the same executable (even if it's just something like --version handling).

We probably won't use this for other packages. It's really just a one-off fix for a persistent issue with Scarab specifically. In the unlikely event that someone reuses this script I'd expect they'd make sure to deal with non-GUI situations if relevant.

On my laptop I'm using gnome+wayland, and 192 dpi. For some reason, gsettings returns uint32 0, and xrdb reports 192. Because the check for 0 is after the xrdb block, it never actually tries to run xrdb, and launches without a scaling setting. Perhaps the 0 check should be specific to gsettings, since it's a documented default there.

Thanks! I've moved the check.

l0b0 requested a review from huantianad October 14, 2024 04:25

l0b0 force-pushed the scarab-scaling branch from f8e3b37 to 636f5c6 Compare October 14, 2024 04:36

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Oct 14, 2024

huantianad approved these changes Oct 14, 2024

View reviewed changes

pkgs/tools/games/scarab/default.nix Outdated Show resolved Hide resolved

l0b0 force-pushed the scarab-scaling branch from 636f5c6 to f7bc5f0 Compare October 14, 2024 08:08

l0b0 requested a review from huantianad October 14, 2024 08:08

l0b0 requested review from Artturin and corngood October 20, 2024 04:42

corngood reviewed Nov 5, 2024

View reviewed changes

l0b0 force-pushed the scarab-scaling branch from f7bc5f0 to 0af8f05 Compare November 6, 2024 00:00

l0b0 requested a review from corngood November 6, 2024 00:00

nix-owners bot requested review from Mic92 and zowoq November 6, 2024 00:01

corngood reviewed Nov 6, 2024

View reviewed changes

l0b0 force-pushed the scarab-scaling branch 2 times, most recently from c18e01e to 7b09fac Compare November 10, 2024 00:36

l0b0 requested a review from corngood November 10, 2024 00:37

corngood approved these changes Nov 10, 2024

View reviewed changes

scarab: Remove unused inputs

b49da1f

l0b0 force-pushed the scarab-scaling branch from 7b09fac to 2e54621 Compare November 10, 2024 00:56

corngood self-requested a review November 10, 2024 01:18

l0b0 force-pushed the scarab-scaling branch from 2e54621 to 59ed3fa Compare November 10, 2024 01:57

corngood approved these changes Nov 10, 2024

View reviewed changes

corngood merged commit 34ed0c9 into NixOS:master Nov 10, 2024
27 of 28 checks passed

l0b0 deleted the scarab-scaling branch November 10, 2024 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scarab: Apply scaling factor in Wayland #348427

scarab: Apply scaling factor in Wayland #348427

l0b0 commented Oct 14, 2024 •

edited

Loading

huantianad left a comment

l0b0 commented Oct 14, 2024

corngood commented Oct 14, 2024

huantianad commented Nov 5, 2024

corngood commented Nov 5, 2024

l0b0 commented Nov 5, 2024 •

edited

Loading

corngood Nov 5, 2024

corngood Nov 5, 2024

l0b0 Nov 6, 2024

corngood Nov 5, 2024

l0b0 Nov 5, 2024 •

edited

Loading

huantianad commented Nov 5, 2024

l0b0 commented Nov 6, 2024

corngood Nov 6, 2024

corngood Nov 6, 2024

corngood left a comment

l0b0 commented Nov 10, 2024 •

edited

Loading

corngood commented Nov 10, 2024

corngood commented Nov 10, 2024

l0b0 commented Nov 10, 2024 •

edited

Loading

corngood commented Nov 10, 2024

l0b0 commented Nov 10, 2024

scarab: Apply scaling factor in Wayland #348427

scarab: Apply scaling factor in Wayland #348427

Conversation

l0b0 commented Oct 14, 2024 • edited Loading

Things done

huantianad left a comment

Choose a reason for hiding this comment

l0b0 commented Oct 14, 2024

corngood commented Oct 14, 2024

huantianad commented Nov 5, 2024

corngood commented Nov 5, 2024

l0b0 commented Nov 5, 2024 • edited Loading

corngood Nov 5, 2024

Choose a reason for hiding this comment

corngood Nov 5, 2024

Choose a reason for hiding this comment

l0b0 Nov 6, 2024

Choose a reason for hiding this comment

corngood Nov 5, 2024

Choose a reason for hiding this comment

l0b0 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

huantianad commented Nov 5, 2024

l0b0 commented Nov 6, 2024

corngood Nov 6, 2024

Choose a reason for hiding this comment

corngood Nov 6, 2024

Choose a reason for hiding this comment

corngood left a comment

Choose a reason for hiding this comment

l0b0 commented Nov 10, 2024 • edited Loading

corngood commented Nov 10, 2024

corngood commented Nov 10, 2024

l0b0 commented Nov 10, 2024 • edited Loading

corngood commented Nov 10, 2024

l0b0 commented Nov 10, 2024

l0b0 commented Oct 14, 2024 •

edited

Loading

l0b0 commented Nov 5, 2024 •

edited

Loading

l0b0 Nov 5, 2024 •

edited

Loading

l0b0 commented Nov 10, 2024 •

edited

Loading

l0b0 commented Nov 10, 2024 •

edited

Loading