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

[WIP] Support wayland with layer shell #1139

Closed
wants to merge 12 commits into from
Closed

Conversation

lbonn
Copy link
Collaborator

@lbonn lbonn commented Jun 5, 2020

As suggested in the discussions of #446, here is an implementation using the layer shell protocol.

I've based this work on the original wayland branch attempt from 2017.

Check list:

  • -normal-window in wayland mode (optional)
  • -monitor in wayland mode (optional)
  • window mode (probably not happening right now)
  • high dpi
  • unify {wayland,xcb}/view.c
  • transparency

As far as I know, this protocol is only supported by wlroots compositors but broader adoption is hopefully on the way. This branch is my daily driver under sway and has been tested briefly on hikari and wayfire.
The code can probably be adapted to use xdg-shell (for example when running with -normal-window) and have basic support on any wayland compositor.

When compiling with wayland enabled, the original xcb implementation is still accessible when running with -x11 or when launched in X11. One difference is that the window mode is not in modi in the default configuration in this case.

About the code: while I tried to remove some duplication it still look a bit messy. Any input on ways to improve the organization would be welcome.

Ah and I've only touched meson, not autoconf. Is there a plan to only keep one of these two?

@emersion
Copy link

emersion commented Jun 6, 2020

As far as I know, this protocol is only supported by wlroots compositors but broader adoption is hopefully on the way.

FWIW, Mir supports it too, and KDE has expressed interest in using it.

@lbonn lbonn force-pushed the wayland-new branch 4 times, most recently from 7729b62 to 87c6239 Compare June 11, 2020 11:03
@lbonn lbonn force-pushed the wayland-new branch 3 times, most recently from a2b9d2d to 9614a5d Compare June 11, 2020 15:52
@lbonn
Copy link
Collaborator Author

lbonn commented Jun 11, 2020

I've simplified some code to remove duplication to a minimum and added support for high dpi (getting scale factors from the compositors, not using the -dpi option).

Please have another look at the final result if you can (git diff next) and let me know if you still see some issues to overcome to consider merging it (at least after the next release is out, I suppose), besides style or documentation matters.

@DaveDavenport
Copy link
Collaborator

From a user:

@qballcow segfaults here (probably related to me using nix to get the deps though)
dies when trying to cleanup proxy in display_cleanup
rofi.c:1144 calls cleanup if the display setup failed, so that obviously cant work

@lbonn
Copy link
Collaborator Author

lbonn commented Jun 24, 2020

I couldn't find any problem calling cleanup() from line 1144 if the display setup failed.
However, it could segfault after setlocale but it should be fixed now.

If the problem persists I can try to reproduce it with more details (setup, nix derivation...).

@ralsei
Copy link

ralsei commented Jun 24, 2020

I can reproduce a segfault upon Esc with rofi -show drun with the following derivation:

default.nix:

{ stdenv, lib, fetchFromGitHub
, pkgconfig, libxkbcommon, pango, which, git, wayland-protocols
, cairo, libxcb, xcbutil, xcbutilwm, xcbutilxrm, libstartup_notification
, bison, flex, librsvg, check, meson, ninja, wayland }:
stdenv.mkDerivation rec {
  pname = "rofi-wayland-unwrapped";
  version = "unstable";

  src = fetchFromGitHub {
    owner = "lbonn";
    repo = "rofi";
    rev = "1806903a29b06acefee9d333c2a4ae44b9d702a2";
    sha256 = "1f1p3d4y37phlyfa4f5wv0mgq35g57ahqc6xqynq2433gisxwir8";
    fetchSubmodules = true;
  };

  preConfigure = ''
    patchShebangs "script"
    # root not present in build /etc/passwd
    sed -i 's/~root/~nobody/g' test/helper-expand.c
  '';

  nativeBuildInputs = [ meson ninja pkgconfig ];
  buildInputs = [ libxkbcommon pango cairo git bison flex librsvg check
    libstartup_notification libxcb xcbutil xcbutilwm xcbutilxrm which
    wayland-protocols wayland
  ];

  doCheck = false;

  meta = with lib; {
    description = "Window switcher, run dialog and dmenu replacement";
    homepage = "https://github.com/davatorium/rofi";
    license = licenses.mit;
    maintainers = with maintainers; [ mbakke ];
    platforms = with platforms; linux;
  };
}

wrapper.nix:

{ symlinkJoin, lib, makeWrapper, hicolor-icon-theme
, pkgs, theme ? null, plugins ? [] }:
let
  rofi-wayland-unwrapped = pkgs.callPackage (import ./default.nix) {};
in symlinkJoin {
  name = "rofi-wayland-${rofi-wayland-unwrapped.version}";

  paths = [
    rofi-wayland-unwrapped.out
  ] ++ (lib.forEach plugins (p: p.out));

  buildInputs = [ makeWrapper ];
  preferLocalBuild = true;
  passthru.unwrapped = rofi-wayland-unwrapped;
  postBuild = ''
    rm -rf $out/bin
    mkdir $out/bin
    ln -s ${rofi-wayland-unwrapped}/bin/* $out/bin

    rm $out/bin/rofi
    makeWrapper ${rofi-wayland-unwrapped}/bin/rofi $out/bin/rofi \
      --prefix XDG_DATA_DIRS : ${hicolor-icon-theme}/share \
      ${lib.optionalString (plugins != []) ''--prefix XDG_DATA_DIRS : ${lib.concatStringsSep ":" (lib.forEach plugins (p: "${p.out}/share"))}''} \
      ${lib.optionalString (theme != null) ''--add-flags "-theme ${theme}"''} \
      ${lib.optionalString (plugins != []) ''--add-flags "-plugin-path $out/lib/rofi"''}

    rm $out/bin/rofi-theme-selector
    makeWrapper ${rofi-wayland-unwrapped}/bin/rofi-theme-selector $out/bin/rofi-theme-selector \
      --prefix XDG_DATA_DIRS : $out/share
  '';

  meta = rofi-wayland-unwrapped.meta // {
    priority = (rofi-wayland-unwrapped.meta.priority or 0) - 1;
  };
}

Running NixOS 20.03 and Sway.

@lbonn
Copy link
Collaborator Author

lbonn commented Jun 28, 2020

@ralsei Thanks!

I've spawned a qemu image running nixos 20.03 but could not reproduce the segfault.

Roughly followed this guide to build the derivation: https://sandervanderburg.blogspot.com/2014/07/managing-private-nix-packages-outside.html.

Sway, rofi and everything were running with default configuration.

This default.nix can be used to spawn a nix-shell --pure and compile the application with debug symbols (meson build; ninja -C build):

with import <nixpkgs> {};
with xorg;

stdenv.mkDerivation {
  name = "rofi-wayland";
  nativeBuildInputs = [ meson pkgconfig cmake ];
  buildInputs = [ libxkbcommon
                  pango
                  cairo
                  git
                  bison
                  flex
                  librsvg
                  check
                  libstartup_notification
                  libxcb
                  ninja
                  xcbutil
                  xcbutilwm
                  xcbutilxrm
                  which
                  wayland
                  wayland-protocols
               ];
}

Could you maybe try to get a traceback with gdb?

@justinlovinger
Copy link

@ralsei I just added this Rofi branch as an overlay in NixOS 20.03, using your Nix expression. I didn't change wrapper.nix (or the rofi attribute), just rofi-unwrapped. I can't reproduce your segfault. I didn't try your wrapper.nix.

@lbonn rofi -show window isn't working for me, on Sway. It says "Mode window is not found".

@lbonn
Copy link
Collaborator Author

lbonn commented Jul 12, 2020

@lbonn rofi -show window isn't working for me, on Sway. It says "Mode window is not found".

Yes, that's a known limitation, there is no wm-agnostic way to get a full window list on wayland as far as I know.

As a replacement, I'm using this external program with rofi in dmenu mode under sway https://github.com/lbonn/i3-focus-last/tree/menu-plus.

@emersion
Copy link

You may want to use the wlr-foreign-toplevel-management protocol for this. Maybe it would be better to keep this for a follow-up pull request, to keep this easier to review.

@lbonn
Copy link
Collaborator Author

lbonn commented Jul 14, 2020

After an irc chat, the maintainers declared they would veto any code using wlroots-specific protocols, in particular layer shell, as a matter of principle. The exact reason seems to be that they don't want to encourage a broader adoption of what they consider a bad design, too close to what exists on X11 and too different from another proposal that they made.

This small battle does not really concern me, so I will soon open a simple fork of rofi with the changes from this PR.

@emersion
Copy link

After an irc chat, the maintainers declared they would veto any code using wlroots-specific protocols, in particular layer shell, as a matter of principle. The exact reason seems to be that they don't want to encourage a broader adoption of what they consider a bad design, too close to what exists on X11 and too different from another proposal that they made.

wlr-layer-shell is not specific to wlroots. implementations exist in Mir and in a Weston fork. KDE is definitely interested in implementing it as well. layer-shell is a candidate for inclusion in the ext namespace of wayland-protocols.

If rofi vetos layer-shell, that just means it won't ever properly work on Wayland, sadly (since e.g. GNOME isn't interested in allowing third-party clients to draw shell UI components).

@ddevault
Copy link

The battle against layer shell has long, long since past. It's either layer shell or no Wayland support for rofi. Let your tired political nonsense die already, @sardemff7

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Jul 15, 2020

After an irc chat, the maintainers declared they would veto any code using wlroots-specific protocols, in particular layer shell, as a matter of principle. The exact reason seems to be that they don't want to encourage a broader adoption of what they consider a bad design, too close to what exists on X11 and too different from another proposal that they made.

This small battle does not really concern me, so I will soon open a simple fork of rofi with the changes from this PR.

small correction, because this does not seem to match what I said on IRC:

I (I can only speak for myself) never said I would veto anything. I just mentioned that I do not have knowledge of wayland, the used protocols etc. and therefor I feel not comfortable to make a decision on the topic. I am currently very busy, so I don't have the time to dive into the topic.

IRC quotes

DaveDavenport | i don't know what is wise.. I know exactly 0 about wayland.
DaveDavenport | It does not feel I have the knowledge to judge this
DaveDavenport | I am mostly very busy atm, and as I said before I find it hard to form an opinion about this, as I cannot judge it.

@lbonn
Copy link
Collaborator Author

lbonn commented Jul 15, 2020

@DaveDavenport Sorry if you feel I misrepresented your position. I will try to develop a bit further. There is no question that the problem is not that the review is taking too long (don't really mind waiting really).

What I understood is that you trusted the judgement of @sardemff7 on this matter at the moment and he threatened to leave the project if it supported layer shell. He also said he left the final decision to you but to be honest I have the impression this was done to keep the question in an uncertain limbo state.

De facto, this is blocked forever and, again, this is your right as maintainers. I was just a bit surprised as this situation was not clear to me at all by simply reading the existing public resources related to the project. Basically what I read was that maintainers were not interested in doing it themselves and asked people to stop asking them for this (perfectly understandable).

Hopefully, there is no hard feeling, I just wanted to clarify the situation for the posterity.

@ddevault
Copy link

@sardemff7 hasn't written any rofi code in over a year so that threat seems a bit hollow.

@DaveDavenport
Copy link
Collaborator

DaveDavenport commented Jul 15, 2020

@sardemff7 has been and is a great support for me in this project. Him leaving because of this is not acceptable for me.

I find it sad this discussion needs to take this tone.

I would like to find a solution that will work for everybody..

@ddevault
Copy link

I find it sad this discussion needs to take this tone.

Yeah, so do I. Just make sure the blame points the right direction.

@justinlovinger
Copy link

@lbonn I am interested in a Wayland fork. It could be called rofi-wlr.

@DaveDavenport It sounds like Wayland support in mainline Rofi is not feasible right now. If the situation changes, a merge with the Wayland fork might be possible.

@DaveDavenport
Copy link
Collaborator

It feels like having a rofi fork for wayland might be the right solution for now.

I would like to make sure we keep in 'sync'.

@lbonn
Copy link
Collaborator Author

lbonn commented Jul 17, 2020

Sorry there were some misunderstanding (I'm in part to blame). Maintainers would still be ok for this to land when they have more review bandwidth and if we manage to craft a cleaner, less coupled architecture.

So in the middle term I also agree we would have to keep a fork (as I believe the feature is already usable). I'll host something on my account in the following days and will close this PR to limit noise on this repo when it's ready.

@markstos
Copy link

I'm glad to hear that a option for Wayland support is in the works. Please leave a comment here when it's ready.

@lbonn
Copy link
Collaborator Author

lbonn commented Jul 24, 2020

Here it is, in the default branch named wayland: https://github.com/lbonn/rofi

@lbonn lbonn closed this Jul 24, 2020
@ddevault
Copy link

Wow, that was remarkably easy and straightforward, what a great candidate for upstreaming

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2020
@lbonn lbonn deleted the wayland-new branch August 23, 2022 22:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants