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.

Sign up for GitHub

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

Jump to bottom

zathura: keep wrapper's WM_CLASS consistent with unwrapped binary #44909

Merged
merged 2 commits into from Dec 10, 2018
Merged

zathura: keep wrapper's WM_CLASS consistent with unwrapped binary #44909

merged 2 commits into from Dec 10, 2018

Conversation

mnacamura
Copy link
Contributor

@mnacamura mnacamura commented Aug 11, 2018 •
edited

Motivation for this change

zathura's WM_CLASS property is inconsistent with what it was in the unwrapped zathura.

$ xprop | grep WM_CLASS
WM_CLASS(STRING) = ".zathura-wrapped_", ".zathura-wrapped_"

This makes some trouble when, in my case, adding a window rule for zathura in bspwm. I expected the following rule works:

bspc rule --add Zathura state=tiled

But actually I needed to do this:

bspc rule --add .zathura-wrapped_ state=tiled
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@mnacamura
Copy link
Contributor Author

This might be a more general issue beyond zathura. Is it better to change wrapProgram to not rename binary?

@mnacamura
Copy link
Contributor Author

Related #31720

@mnacamura
Copy link
Contributor Author

This might be a more general issue beyond zathura. Is it better to change wrapProgram to not rename binary?

Ahh I see it's difficult... 9deb7f8#diff-9b66ee4614cafec580d4df291629ef47 76f4eb5#diff-9b66ee4614cafec580d4df291629ef47

@mnacamura mnacamura changed the title zathura: keep wrapper's WM_CLASS consistent with wrapped binary zathura: keep wrapper's WM_CLASS consistent with I unwrapped binary Aug 12, 2018
@mnacamura mnacamura changed the title zathura: keep wrapper's WM_CLASS consistent with I unwrapped binary zathura: keep wrapper's WM_CLASS consistent with unwrapped binary Aug 12, 2018
@jtojnar
Copy link
Contributor

jtojnar commented Sep 2, 2018

This is probably an issue with Zathura. They need to set WM_CLASS: https://wiki.gnome.org/Projects/GnomeShell/ApplicationBased#The_WM_CLASS_X_Window_property

@mnacamura
Copy link
Contributor Author

In share/applications/org.pwmt.zathura.desktop, they set Name=Zathura and Exec=zathura. In main.c, they call g_option_context_parse(): https://git.pwmt.org/pwmt/zathura/blob/master/zathura/main.c#L170-176. Is it not sufficient?

@veprbl
Copy link
Member

veprbl commented Dec 8, 2018

@GrahamcOfBorg build zathura

veprbl
veprbl approved these changes Dec 8, 2018
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

From what I understand, you remove binaries of zathura_core by separating them into a separate output bin, which you later omit in the paths for symlinkJoin. The remaining parts are separated into three outputs man, dev and out, but they don't do much for the user because zathura_core is not exposed at the moment. You could instead do rm "$out/bin/zathura" in postBuild of the wrapper and then makeWrapper the same way as you do now. Your way appears to me to be a bit implicit, but also it is a bit less hacky (no making and then deleting a symlink).

The other thing that you do is you remove one layer of wrappers, which renders zathura_core potentially unable to locate file, but this should be fine as long as we don't expose zathura_core to the users.

Overall, I think, this PR is good and it does solve the problem.

@veprbl veprbl merged commit 6fb67ca into NixOS:master Dec 10, 2018
@mnacamura
Copy link
Contributor Author

Thank you for merging!

@mnacamura mnacamura deleted the zathura branch December 11, 2018 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl veprbl approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mnacamura @jtojnar @veprbl @GrahamcOfBorg