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

home-cursor.nix: enable gtk module when enabling gtk config generation #4144

Merged
merged 4 commits into from
Jul 14, 2023
Merged

home-cursor.nix: enable gtk module when enabling gtk config generation #4144

merged 4 commits into from
Jul 14, 2023

Conversation

Maroka-chan
Copy link
Contributor

The gtk configurations are not generated unless config.gtk is enabled. This is a point of confusion because config.home.pointerCursor.gtk can essentially be disabled, despite having it enabled.

Description

I changed the home.pointerCursor.gtk option such that it also enables the gtk module. The pointerCursor.gtk option essentially does nothing when the gtk module is disabled, so I think it is safe to assume that you would want to enable gtk when using this option. My cursor size was not consistent between applications, and it admittedly took me way too long to figure out that this was the problem..

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

The gtk configurations are not generated unless config.gtk is enabled.
This is a point of confusion because config.home.pointerCursor.gtk can essentially be disabled,
despite having it enabled.
@ncfavier
Copy link
Member

@polykernel what do you think?

@polykernel
Copy link
Contributor

Sorry for the extremely late response, some real life stuff got in the way.

The gtk configurations are not generated unless config.gtk is enabled. This is a point of confusion because config.home.pointerCursor.gtk can essentially be disabled, despite having it enabled.

I agree that this behavior can be confusing for users, but enabling the gtk module implictly extends the scope of home.pointerCursor. The sole responsibility of the home.pointerCursor module is to generate cursor configurations. When the option home.pointerCursor.<backend> is enabled, the outcome should be that the cursor configuration for that particular backend is generated. The idea is to separate generation of the configuration from activation and thereby make activation explicit.

For example, suppose an user has the following block in their home-manager configuration

...
gkt.enable = true;
home.pointerCursor.gtk.enable = true;
...

If the user wants to disable all gtk configurations, then commenting out gtk.enable suffice. However, if home.pointerCursor.gtk also contained logic for activating the gtk module, then gtk.enable no longer serves as the master switch and this can potentially leads to some subtle bugs(one can argue using mkOverride would solve the problem but this just shifts the level where it occurs since then the mkOverride priority essentially becomes the default priority).

Therefore I think it is important to preserve the separation between generating and activating the configuration even though it is inconvenient here. The documentation for the module should be updated to indicate this.

@rycee
Copy link
Member

rycee commented Jul 8, 2023

Agreed, can just add some text in the home.pointerCursor.gtk.enable to indicate that it only applies if gkt.enable is enabled.

@Maroka-chan
Copy link
Contributor Author

That makes sense. I added a note to the option description instead. It's hopefully clear that it refers to the config.gtk option.

@@ -42,7 +42,8 @@ let

gtk = {
enable = mkEnableOption ''
gtk config generation for <option>home.pointerCursor</option>
gtk config generation for <option>home.pointerCursor</option>.
Note that <option>gtk</option> has to be enabled for the configs to take effect
Copy link
Contributor

@polykernel polykernel Jul 10, 2023

Choose a reason for hiding this comment

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

It may be worthwhile to add a clause under the main home.pointerCursor submodule's description since the same requirement holds for the x11 backend and xsession.enable.

Perhaps something akin to

description = ''
  Cursor configuration. Set to <literal>null</literal> to disable.
  </para><para>
  Top-level options declared under this submodule are backend independent
  options. Options declared under namespaces such as <literal>x11</literal>
  are backend specific options. By default, only backend independent cursor
  configurations are generated. If you need configurations for specific
  backends, you can toggle them via the enable option. For example,
  <xref linkend="opt-home.pointerCursor.x11.enable"/>
  will enable x11 cursor configurations.
  </para><para>
  Note that the cursor configurations are only generated, the relevant
  subsytems still must be configured for them to take effect. For example,
  <xref linkend="opt-home.pointerCursor.gtk.enable"/> will enable
  gtk cursor configuration but <xref linkend="opt-gtk.enable"/> needs
  to be set for it to be applied.
'';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I've now removed the note on the gtk option and added the more general note under the main submodule description :)

Comment on lines 107 to 112
</para><para>
Note that this will merely generate the cursor configurations.
To apply the configurations, the relevant subsytems must also be configured.
For example, <xref linkend="opt-home.pointerCursor.gtk.enable"/> will generate
the gtk cursor configuration, but <xref linkend="opt-gtk.enable"/> needs
to be set for it to be applied.
Copy link
Member

Choose a reason for hiding this comment

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

You seem to have mixed spaces and tabs here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh shoot, you're right. I haven't gotten around to configure vim on this machine yet, so I didn't consider that it made tabs. In any case, It should be consistent now.

Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

macos tests should succeed once nixpk.gs/pr-tracker.html?pr=242954 turns all green

@ncfavier ncfavier merged commit d2e47de into nix-community:master Jul 14, 2023
antholeole pushed a commit to antholeole/home-manager that referenced this pull request Jul 24, 2023
nix-community#4144)

* home-cursor.nix: enable gtk module when enabling gtk config generation

The gtk configurations are not generated unless config.gtk is enabled.
This is a point of confusion because config.home.pointerCursor.gtk can essentially be disabled,
despite having it enabled.

* home-cursor.nix: Add note to gtk config generation description instead of enabling gtk module

* home-cursor.nix: Add note about applying pointerCursor configs to main submodule desc

* home-cursor.nix: Change tabs to spaces
SimSaladin added a commit to SimSaladin/home-manager that referenced this pull request Aug 26, 2023
* upstream/master: (352 commits)
  flake.lock: Update
  qcal: add module
  bat: generate cache file in XDG cache home
  nixos: only refer to `users.users` if needed
  home-cursor: remove IFD when linking icon directories
  flake.lock: Update
  home-cursor: improve icon compatibility
  fix(qt): allow theming for apps started by systemd (nix-community#4349)
  flake.lock: Update
  fcitx5: add `fcitx5-config-qt` to test stub
  pqiv: only run tests on Linux platforms
  pqiv: add module
  Translate using Weblate (German)
  xplr: add module
  browserpass: support librewolf
  Translate using Weblate (Romanian)
  Translate using Weblate (Portuguese (Brazil))
  hyprland: remove xwayland.hidpi (nix-community#4302)
  home-manager: handle profile list in Nix >2.17
  lf: simplify option validation (nix-community#4334)
  programs.khal: uncomment locale config (nix-community#4319)
  kanshi: support adaptive sync (nix-community#4328)
  mu: add package option (nix-community#4325)
  modules: types.string throws error now (nix-community#4324)
  taffybar: Avoid restarting too quickly (nix-community#4316)
  aerc: do not use smtp-starttls (nix-community#4272)
  zsh: Add `zsh.history.ignoreAllDups` config option (nix-community#4248)
  jujutsu: fix zsh completion (nix-community#4305)
  Translate using Weblate (Korean)
  Translate using Weblate (Romanian)
  Translate using Weblate (German)
  flake.lock: Update
  network-manager-applet: remove `--sm-disable` flag
  himalaya: update a link to ticket
  nushell: deprecation of let-env (nix-community#4292)
  man: fix caches generation in cross-compiled system (nix-community#4294)
  fish: fix session vars build in cross-compiled system (nix-community#4293)
  Update translation files
  Translate using Weblate (Chinese (Simplified))
  gh: option to enable helper for additional hosts (nix-community#4288)
  home-manager: rework news command
  home-manager: skip manual in uninstall configuration
  gnome-terminal: add assertion on profile names
  hyprland: use `toKeyValue`'s `indent` argument (nix-community#4274)
  gpg: fix typo (nix-community#4277)
  firefox: make package nullable (nix-community#4113)
  git-sync: add news entry for darwin
  tmate: don't generate empty config file (nix-community#4271)
  gh-dash: add module
  git-sync: add darwin support
  flake.lock: Update
  hyprland: prioritize variables and beziers (nix-community#4263)
  hyprland: add module
  script-directory: fix documentation link (nix-community#4258)
  Translate using Weblate (Indonesian)
  Translate using Weblate (Ukrainian)
  Translate using Weblate (Spanish)
  Translate using Weblate (Swedish)
  docs: hide `_module.*` from NixOS/nix-darwin docs
  jujutsu: update for Jujutsu 0.8.0 (nix-community#4250)
  ripgrep: don't set env. variable if no config (nix-community#4254)
  docs: update for Markdown migration
  treewide: remove now-redundant `lib.mdDoc` calls
  docs: clean up after Markdown migration
  flake: remove temporary nixpkgs pin
  treewide: convert all option docs to Markdown
  treewide: adjust some DocBook for conversion
  treewide: `mkAliasOption` -> `mkAliasOptionMD`
  treewide: `mkPackageOption` -> `mkPackageOptionMD`
  treewide: convert options with tables to Markdown
  treewide: convert options with lists to Markdown
  treewide: convert custom `enable` docs to Markdown
  treewide: convert parameterized docs to Markdown
  treewide: manually convert some docs to Markdown
  treewide: fix `mkEnableOption` arguments
  treewide: add missing option descriptions
  docs: use `nixosOptionsDoc`
  flake: temporarily pin nixpkgs version
  version: add `isReleaseBranch`
  manual: add test
  Update translation files
  flake.lock: Update
  home-manager: remove Home Manager default paths
  imapnotify: Use JSON type for extraConfig (nix-community#4238)
  Translate using Weblate (French)
  zsh: fix custom syntax highlighting styles (nix-community#4236)
  i3-sway: allow arbitrary floating modifier (nix-community#4229)
  i3-sway: multiple outputs (nix-community#4223)
  home-cursor.nix: enable gtk module when enabling gtk config generation (nix-community#4144)
  aerc: add assertion to limit per-account extraConfig to UI config (nix-community#4196)
  flake.lock: Update
  ssh-agent: add assertion and fix news entry (nix-community#4210)
  imapnotify: move test
  imapnotify: use direct nix store path for config
  flake.lock: Update
  nushell: add login.nu configuration option
  himalaya: fix notmuch backend
  swayosd: add module
  pyenv: add module
  darcs: add module
  tests: some minor cleanups
  goimapnotify: remove test dependency on notmuch
  unison: Allow using same option multiple times (nix-community#4208)
  imapnotify: Add launchd agent (nix-community#3291)
  i3: remove deprecated example in i3.config.startup (nix-community#4201)
  antidote: static file move to /tmp
  Translate using Weblate (Indonesian)
  PULL_REQUEST_TEMPLATE.md: add maintainer cc section (nix-community#4193)
  programs.khal
  vdirsyncer: synchronize metadata as well (nix-community#4167)
  i18n: Use glibcLocales from NixOS if possible (nix-community#2333) (nix-community#4177)
  xfconf: remove properties with null values (nix-community#4192)
  home-manager: Use `path:` URI type for flake default (nix-community#3646)
  ci: autolabel calendars PRs (nix-community#4165)
  flake.lock: Update (nix-community#4161)
  news: fix typo in programs.zsh.antidote entry
  ssh-agent: init module (nix-community#4178)
  chromium: fix `commandLineArgs` to use the user specified package (nix-community#4175)
  sxhkd: allow usage of derivations as keybind commands (nix-community#4169)
  README: Remove the pills (nix-community#4181)
  lsd: use -A instead of -a in aliases (nix-community#4173)
  kakoune: add defaultEditor option
  docs: update link to allowed users setting (nix-community#4176)
  zsh: allow setting custom syntax highlighting styles (nix-community#4122)
  flake: add formatter (nix-community#3620)
  broot: fix test (nix-community#4170)
  antidote: fix .dot path
  ci: build manual and push to home-manager.dev
  ...
@Maroka-chan Maroka-chan deleted the enable-gtk-in-pointerCursor branch September 9, 2023 21:14
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.

4 participants