Add graphical display chooser #59
Conversation
|
Nice, this will be useful. Wondering if we should default to slurp if it is available and no CLI arg is specified. Also wondering if we can/should pass the whole chooser command as an argument (or some other way, config file even?) so we aren't limited to wofi/bemenu/slurp. Overall, we should eliminate as many CLI args as possible as this is autolaunched by dbus, and editing service files is bound to create issues for people who don't understand what to do. |
|
I thought about a environment variable to set the chooser, since you have to set other env variables before launching sway or other compositors. Or you could do that in your compositor. Do you have an idea, where to place the config file? XDG_CONFIG_HOME/xdg-desktop-portal-wlr/config, maybe? It would be possible to allow an custom command for dmenu type choosers (list over stdin) or one for slurp like (no stdin) afaik, since i don't know how to differentiate those and sent something/nothing to stdin at forktime. But yes, a config file could contain a slurp on/off flag and a custom dmenu command. |
|
We do already need to set XDG_CURRENT_DESKTOP, but that creates a lot of (sadly unavoidable) confusion amongst users due to the complicated way d-bus activated programs receive their environment from systemd user sessions. We would be somewhat compounding the problem by adding more environment variable based config. Also, i can think of more config file uses down the road, like dmabuf export/dmabuf copying/cpu based screencopy. If we were to go the config file route, I would say it should live in As far as customizable commands, I was thinking of allowing the user to supply a full, arbitrary command string where we could use templating to enumerate the various display choices, or suggest that each command would receive that list on STDIN and let the users create a pipeline that works for the program of their choosing. While bemenu/dmenu/wofi/slurp seem like the obvious choices, I'd hate to limit users to only those four in case they have different ideas for selection. Wayfire users, for instance, may not find those tools fit in with a non-tiling workflow. |
|
ok, then my suggestion would be, to add a chooser type option (dmenu and standalone). If choosen dmenu, we will send a '\n' seperated list of all Displaynames to STDIN of the command and in case of standalone the chooser has to get his own list of Displays (wlroots api, sway-ipc, etc.) and we just expect it to return the name of the display. |
|
We don't need to implement everything in a single pull request. Sticking with basic functionality (e.g. hardcoded picker) would ease review and help move forward quicker. |
|
That's a good point. |
columbarius
force-pushed
the
dmenu-chooser
branch
from
8a9337a to
e0ec840
Compare
|
I did a little bit of cleanup, like pulling the xdpw_wlr_output_first function inside the chooser and renaming the slurp functions to simple. This will let us create the output chooser types simple and dmenu. |
columbarius
force-pushed
the
dmenu-chooser
branch
from
d02736e to
500af4c
Compare
|
Added output chooser struct and simplified the default_chooser mechanism, to better accommodate an output chooser option in a config file. So from my side, this would be ready for testing and merging. |
|
I'll build this branch and start using it for the next week and see if I have any issues. You may want to ask others to do the same in the original issue about selecting outputs. |
|
So the slurp chooser doesn't seem to work for me, but maybe something is messed up on my side. Seems like it is getting skipped. I'll look into it and report back. Edit: bemenu as a fallback choice does seem to function as intended. I don't have wofi installed anymore. Edit2: I hadn't updated slurp yet, but now that I have, it still disregards my slurp selection and falls back to bemenu anyway. Edit3: Huh, this is weird, if I run the slurp example |
|
Interesting. Could you try e0ec840? I don't know, if there is a difference between exec slurp directly, or trough sh. |
Haven't tried the e0ec840 commit yet. It definitely runs slurp, because I see the selector. Display names are DP-1 (the external) and eDP-1 (the integrated display, it's a laptop). |
|
Does this helps with debugging? |
Yes, that'll help. I'll give this a shot a little later. |
|
Here's a log of it skipping past slurp. Looks like slurp may be appending some strange extra characters to the display name. Edit: Maybe we need to quote the format string? Or maybe there's an off-by-one error somewhere? Edit2: Also, bemenu works fine for me. I just slapped escape this time, which I presume is why it returned garbage. |
|
Could you try that please? I don't know how to debug it, since I can't reproduce those errors. Do you have an idea? On press escape I also receive garbage. |
src/screencast/wlr_screencast.c
Outdated
| close(p1[0]); | ||
| close(p2[1]); | ||
| close(p1[1]); | ||
| if (read(p2[0],name,name_maxlength*sizeof(char)) == -1) { |
There was a problem hiding this comment.
I think we can replace this with something among those lines:
FILE *f = fdopen(p2[0]);
char *name = NULL;
size_t size = NULL;
ssize_t nread = getline(&name, &size, f);
fclose(f);
if (nread < 0) {
perror("getline failed");
return false;
}
src/screencast/wlr_screencast.c
Outdated
| int p2[2]; //c -> p | ||
|
|
||
| if (pipe(p1) == -1) { | ||
| logprint(ERROR,"Failed to open pipe"); |
There was a problem hiding this comment.
Style: missing space after comma (applies to the whole patch)
| } | ||
|
|
||
| struct xdpw_wlr_output *wlr_output_chooser_simple(char *cmd, struct wl_list *output_list) { | ||
|
|
There was a problem hiding this comment.
Style: unnecessary newline (applies to the whole patch)
src/screencast/wlr_screencast.c
Outdated
| int i = 0; | ||
|
|
||
| wl_list_for_each_safe(output, tmp, output_list, link) { | ||
| namelength = (namelength > strlen(output->name)) ? namelength : strlen(output->name); |
There was a problem hiding this comment.
getline would allow us to get rid of this logic.
src/screencast/wlr_screencast.c
Outdated
| } | ||
| namelength++; | ||
|
|
||
| char buffer[maxlength]; |
There was a problem hiding this comment.
It would be easier to not build a buffer, and instead directly write the output names to the pipe.
|
@emersion thanks. Your getline input pushed me to take a deeper look into unix string, stream and file handling. I unified both chooser with an optional tmpfile replacing the buffer. If that's a better solution, I would cleanup the xdpw_wlr_output_chooser function. |
Instead of using a tempfile, couldn't you just continue writing the outputs directly to the STDIN pipe? We could have a boolean arg to chooser_universal that specifies whether or not we should supply the output list on STDIN. |
|
@danshick thanks! Havn't seen pipes created outside of the forked function, but that's totally possible. Great idea! |
|
ok, now testing can begin. |
|
I've tested this now on Arch Linux. Here are the testing steps I took and the result. Testing stepsFirst, I the steps to build from source didn't work because an iniparser library was missing. It looks like maybe this dependency was added to support the config file added by this PR. If so, the README should be updated the recommended route to resolve this. On Arch Linux, installing the
From there:
ResultI have First, the names presented were not friendly, they were like "DP-1" and "eDP-2". It was not clear which one of those was the external monitor. I would expect the make/model/serial to be included as well. Second, after selecting an output, nothing happened and there was no output emitted by the xdpw command. To be sure the problem wasn't from having a single-person meeting, I joined the meeting from my phone as well and tried again but still my screen did not appear to be shared. |
This would be a good improvement, but let's not address this yet. Let's keep it for a follow-up patch. |
|
Thanks for your testing!
Good point. The problem is that the monitor name (like "DP-1") is unique, the model is not and combining monitor name and monitor is more complicated than needed and slurp only return the monitor name. I would suggest keeping the api from our side, but adding a script to contrib, which queries the models to the corresponding names, creating a nicer dmenu list and translating it back afterwards. This should be easier and more robust.
Could you please start xdg-desktop-portal-wlr manually with: and provide the log? |
We could do something like |
|
So far so good. Still working well. I'll let you know if anything buggy comes up. |
|
@columbarius Here are the debug logs: |
|
@danshick How did you test it? |
|
@markstos, are you sure your setup works fine without this PR? It doesn't seem like this issue is related to this PR. |
|
@emersion No I'm not sure. I'll do more testing. |
Are you on Arch? If so, can you post the output of |
|
I'm also on Arch, and tested via this package: https://aur.archlinux.org/packages/xdg-desktop-portal-wlr-chooser-git/ Didn't have much success. I think I was given a choice of desktop (one was slightly dimmer than the other as I'd move my mouse across them), but once I clicked I didn't see any shared screen. Going back to the old package now to see if sharing itself still works. |
Can you also run the command I shared above? |
|
Back on the old package, and sharing is not working at all 🤔 Looks like I really need to get Aura's |
|
@fosskers ...and then restart pipewire and try again. I suspect this is the problem both of you are having. It has nothing to do with this PR. Just a recent update issue. |
|
Yup, using the pacnews fixed it! Thank you. |
|
didn't test that with a second monitor, which breaks it. I will let that to someone with a better bash-foo and created #105 you could use something like this https://gist.github.com/columbarius/1245a030f16c9139c6870576554ee048 |
|
Works like a charm, in Firefox and Chromium (with pipewire 0.3; recent versions of Chromium no longer require libpipewire02), with my usual setup: Is there any specific functionality that requires testing? |
|
Mostly, if I messed something up, when refactoring the requested changes. I had now time to test with a second monitor and replugging one monitor can still lead to a corrupted output_list, see #76. |
Supports "dmenu" chooser type, which is called with a dmenu type list piped to stdin, "simple" type, which recieves nothing on stdin and default, which tries the hardcoded choosers. Choosers are required to return the name of the choosen output as given by the xdg-output protocol. Thanks to piater for closing overlooked pipes. Thanks to ericonr for suggestions regarding fork and pipes.
columbarius
force-pushed
the
dmenu-chooser
branch
from
c863b74 to
6e2712f
Compare
|
squashed all fixup commits, removed '-Werror=maybe-uninitialized', and added the workaround for the corrupted output list. |
|
@columbarius Thanks for your hard work on this and other PRs. |
|
Thanks for your fantastic reviews and thanks for merging! |
This provides the posibility to choose the screencast display via a graphical interface provided either by slurp, wofi or dmenu. If a display is given via, the commandline parameter, it is prioritised, in case all selectors fail, it will fall back to the current default. For slurp to work properly emersion/slurp#64 is required.
Note: while this is not merged and man pages are not ready, the locations for a config file are in order:
The system wide config can be in another place, if SYSCONFDIR differs from /etc