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

waybar: enable experimental features #251115

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Conversation

NotAShelf
Copy link
Member

@NotAShelf NotAShelf commented Aug 24, 2023

This addresses my concerns in #250551 by doing the following:

  • Adds an experimentalPatches option to waybar to optionally enable experimental features of waybar, such as but (probably) not limited to wlr/workspaces

  • Adds a waybar-experimental package that comes with experimentalPatches enabled, mainly to allow caching by hydra instead of forcing users to build it themselves

Without experimentalPatches (a.k.a experimental meson flag), users are forced into using hyprland/workspaces which is, at the moment, a below par replacement to previously standard wlr/workspaces. Additionally, if a single waybar configuration is shared between two separated wlroots compositors the users also left without a viable solution.

Ideally we would want to enable experimental patches by default because why wouldn't we? but I assumed it would be against some obscure nixpkgs idiom I haven't heard about.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@NotAShelf
Copy link
Member Author

CC: @khaneliman @AndersonTorres

@NotAShelf
Copy link
Member Author

Come to think of it, this is not a reliable solution as is. We need to provide the hyprland-specific workspaces patch regardless of experimental patches.

@khaneliman I would like to suggest reverting the removal of waybar-hyprland. It was a breaking change that we cannot quite substitute for.

@NotAShelf
Copy link
Member Author

NotAShelf commented Aug 24, 2023

Okay I think this is good enough to provide a solution to the lack of experimental patches. The lack of hyprland-specific patches can (and I think should) be resolved elsewhere (once again, I think waybar-hyprland should be brought back)

@NotAShelf NotAShelf changed the title waybar: add experimentalPatches option waybar-experimental: init Aug 24, 2023
@NotAShelf NotAShelf force-pushed the master branch 3 times, most recently from eaa848b to c270fd5 Compare August 24, 2023 08:55
@fufexan
Copy link
Contributor

fufexan commented Aug 24, 2023

I'm more in favor of reverting #250551, since Hyprland does not support wlr/workspaces properly at the moment, and, as @NotAShelf said, hyprland/workspaces is not as complete as wlr/workspaces.
I'm in favor of this PR. Hyprland should fully support wlr/workspaces soon.

No offense, but the PR should've been tested by a few more people before being merged.

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Squash the commits into one, with the title

waybar: enable experimental features

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Aug 24, 2023
@ofborg 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 Aug 24, 2023
@NotAShelf NotAShelf force-pushed the master branch 2 times, most recently from 0d0378c to 605ca4a Compare August 24, 2023 10:54
@NotAShelf
Copy link
Member Author

Should be good now.

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

waiting ofborg

@AndersonTorres
Copy link
Member

AndersonTorres commented Aug 24, 2023

(once again, I think waybar-hyprland should be brought back)

Each and every user is free to use the .override { . . . } design pattern.

Also, it would be ridiculous to create a new "package" (i.e. entry on top-level/all-packages.nix) for each 2^19 Boolean flag combinations of this single package.

Just create two or at most three entries for the most common and/or interesting use cases and the rest is left as an exercise to the user.

@NotAShelf
Copy link
Member Author

NotAShelf commented Aug 24, 2023

Each and every user is free to use the .override { . . . } design pattern.

Also, it would be ridiculous to create a new "package" (i.e. entry on top-level/all-packages.nix) for each 2^19 Boolean flag combinations of this single package.

Just create two or at most three entries for the most common and/or interesting use cases and the rest is left as an exercise to the user.

The original idea was that the removal of waybar-hyprland was irreplaceable without a custom patch phase and a rebuild for every Hyprland user whenever waybar's source got bumped. I do understand your point, but it's more beneficial to allow users to benefit from the binary cache if they have to rely on rebuilds to make the program functional.

The implementation of waybar-experimental is more logical as experimental patches are something any user might want to access regardless of their compositor, so I think the experimentalPatches warranting its own package is valid.

@AndersonTorres
Copy link
Member

I do understand your point, but it's more beneficial to allow users to benefit from the binary cache if they have to rely on rebuilds to make the program functional.

An "interesting use case", you say? ◉_◉

How many of them you believe are needed?

  • experimental
  • hyprland
  • both
  • none

I particularly prefer a fat, all-batteries-included as default.

@khaneliman
Copy link
Contributor

Come to think of it, this is not a reliable solution as is. We need to provide the hyprland-specific workspaces patch regardless of experimental patches.

@khaneliman I would like to suggest reverting the removal of waybar-hyprland. It was a breaking change that we cannot quite substitute for.

What do you need from wlr/workspaces that isn't in hyprland/workspaces? The previous waybar-hyprland overlay being from removed from hyprland flake and the version in nixpkgs broke workspace switching to me and i switched to hyprland/workspaces and that worked again. I started upstreaming changes to Alexays/Waybar#2429 to get back feature parity.

@khaneliman
Copy link
Contributor

I'm more in favor of reverting #250551, since Hyprland does not support wlr/workspaces properly at the moment, and, as @NotAShelf said, hyprland/workspaces is not as complete as wlr/workspaces. I'm in favor of this PR. Hyprland should fully support wlr/workspaces soon.

No offense, but the PR should've been tested by a few more people before being merged.

Is the idea that Hyprland will just support straight wlr/workspaces, then and deprecate hyprland/workspaces?

@NotAShelf
Copy link
Member Author

I do understand your point, but it's more beneficial to allow users to benefit from the binary cache if they have to rely on rebuilds to make the program functional.

An "interesting use case", you say? ◉_◉

How many of them you believe are needed?

* experimental

* hyprland

* both

* none

I particularly prefer a fat, all-batteries-included as default.

simply waybar and waybar-experimental. And as I've mentioned above, I would much rather if experimental patches were enabled by default.

@NotAShelf
Copy link
Member Author

NotAShelf commented Aug 24, 2023

I'm more in favor of reverting #250551, since Hyprland does not support wlr/workspaces properly at the moment, and, as @NotAShelf said, hyprland/workspaces is not as complete as wlr/workspaces. I'm in favor of this PR. Hyprland should fully support wlr/workspaces soon.
No offense, but the PR should've been tested by a few more people before being merged.

Is the idea that Hyprland will just support straight wlr/workspaces, then and deprecate hyprland/workspaces?

ideally I would like hyprland/workspaces to be a 1:1 replica of wlr/workspaces with the only difference being the usage of IPC. I depend on wlr/workspaces, yknow, working so that I may share my waybar configuration between sway and hyprland.

Is the idea that Hyprland will just support straight wlr/workspaces, then and deprecate hyprland/workspaces?

We have no control over hyprland/workspaces, and I don't even know what it's supposed to achieve over the usage of IPC. Currently it's just a feature incomplete version of wlr/workspaces.

@NotAShelf
Copy link
Member Author

NotAShelf commented Aug 24, 2023

The implementation of of waybar-experimental solves the problem entirely. It makes wlr/workspaces work and according to Vaxry (the Hyprland developer) Hyprland has been respecting the protocol needed by wlr/workspaces for the past 3 months, without further patching.

edit: just tested and it IN FACT works without further patching.

If @AndersonTorres is against providing two separate packages, then I propose enabling experimental patches by default as they do nothing other than providing extra functionality (and solve our problems.)

@AndersonTorres
Copy link
Member

I have no preference about this here. I am just thinking about the combinatorial explosion.

Why the upstream call that "experimental" to begin with? If such experimental functionality is not api-unstable and not dangerous (on the sense of "too sensitive code, expect buffer overflows"), then I believe there is nothing bad in enabling it by default.

@NotAShelf
Copy link
Member Author

NotAShelf commented Aug 24, 2023

I have no preference about this here. I am just thinking about the combinatorial explosion.

Why the upstream call that "experimental" to begin with? If such experimental functionality is not api-unstable and not dangerous (on the sense of "too sensitive code, expect buffer overflows"), then I believe there is nothing bad in enabling it by default.

I think it is because it tries to call for wayland protocols that are not yet merged, but are expected to be implemented by the compositor. But I can assure you that running "experimental" Waybar has caused no problems over the past year or so, during which I've used Waybar on both Archlinux and NixOS.

@NotAShelf NotAShelf changed the title waybar-experimental: init waybar: enable experimental features Aug 24, 2023
@khaneliman
Copy link
Contributor

The implementation of of waybar-experimental solves the problem entirely. It makes wlr/workspaces work and according to Vaxry (the Hyprland developer) Hyprland has been respecting the protocol needed by wlr/workspaces for the past 3 months, without further patching.

edit: just tested and it IN FACT works without further patching.

If @AndersonTorres is against providing two separate packages, then I propose enabling experimental patches by default as they do nothing other than providing extra functionality (and solve our problems.)

If it works fine with just experimental, I would probably switch back to wlr/workspaces for my configuration again.

@NotAShelf
Copy link
Member Author

If it works fine with just experimental, I would probably switch back to wlr/workspaces for my configuration again.

Seems to work flawlessly, including workspace switching.

@fufexan
Copy link
Contributor

fufexan commented Aug 24, 2023

IMO we can simply enable the experimentalPatches flag by default. No need for waybar-experimental. It provides extra functionality, while not hindering users of compositors that don't implement that protocol. If anyone still wants not to use the experimental patches, they can override.

@NotAShelf
Copy link
Member Author

Alright, I'm going to make the changes then.

additional bool flag to optionally enable experimental features such as wlr/workspaces
@khaneliman
Copy link
Contributor

khaneliman commented Aug 24, 2023

Verified that removing the hyprland patch and just using the experimental dispatcher works with the systemd service, too since we're not introducing out of scope binary commands into code.

@khaneliman
Copy link
Contributor

@AndersonTorres looks like checks finished

@MakiseKurisu
Copy link
Contributor

If you have broken workspace in waybar AGAIN after upgrading your system, please be aware that Hyprland has removed wlr-ext-workspace support. You are recommended to switch back to hyprland/workspaces again.

MakiseKurisu added a commit to MakiseKurisu/nixos-config that referenced this pull request Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants