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

Allow monitor choice #24

Merged
merged 2 commits into from
Jun 3, 2024
Merged

Allow monitor choice #24

merged 2 commits into from
Jun 3, 2024

Conversation

ben-grande
Copy link
Contributor

The client can choose the monitor, which will be prompted in the Qrexec dialog. The monitor can be enforced overridden by the target by environment variable. Useful on a multi-monitor setup, especially when Dom0 is the target.

Fixes: QubesOS/qubes-issues#9275

@ben-grande
Copy link
Contributor Author

Possibly requires #23 to avoid merge conflicts on the DBUS variable.

@marmarek
Copy link
Member

I don't think it should be client selecting the monitor. Besides the policy aspect (who control what is shared), the display names may not match (or be known at all) to the source qube - see xrandr output in VM. And even when known, I'm not sure if they are guaranteed to be compatible with qrexec argument (like if it contains . or maybe there could be :?).
Maybe there could be a zenity (or gtk directy?) prompt to select which monitor on the service side?

Later it could be extended to include also a screenshot, or even selecting individual window (the latter might be more problematic, and may deserve separate separate service if a protocol with dynamic frame size would be needed).

@ben-grande
Copy link
Contributor Author

I don't think it should be client selecting the monitor. Besides the policy aspect (who control what is shared), the display names may not match (or be known at all) to the source qube - see xrandr output in VM.

The monitor names are not known by the source. I just allow the client to specify the target because then it can be shown in the Qrexec dialog, but in any case, the target can always override the monitor name.

You following suggestions seems promising, especially the combination of dialog box with screenshot of the monitors.

And even when known, I'm not sure if they are guaranteed to be compatible with qrexec argument (like if it contains . or maybe there could be :?).

I believe the monitor names can only have these characeters: [0-9A-Za-z].

See the output of:

xrandr | awk '/ (dis)?connected /{print $1}'

I didn't read the xrandr source code to verify and did not find information in the manual page.

Qrexec arguments can have the following characters: [0-9A-Za-z+_.].

Maybe there could be a zenity (or gtk directy?) prompt to select which monitor on the service side?

I thought about this, but then thought there should be less dialogs for UX reasons, but if you prefer a dialog, I can do that instead.

Later it could be extended to include also a screenshot, or even selecting individual window (the latter might be more problematic, and may deserve separate separate service if a protocol with dynamic frame size would be needed).

A screenshot of the screens would be nice, I thought of this to be more explicit but then went in the simple route.

zenity doesn't allow embedding images. yad does and it uses GTK+ behind the scenes. Can be GTK directly also, but that is something I have to learn and examples uses in Qubes for reference would be nice.

@ben-grande
Copy link
Contributor Author

yad does seen to have some dependencies:

Installing:
 yad                    x86_64   9.3-4.fc37          qubes-dom0-cached   235 k
Installing dependencies:
 gspell                 x86_64   1.12.1-1.fc37       qubes-dom0-cached   106 k
 gtksourceview3         x86_64   3.24.11-8.fc37      qubes-dom0-cached   581 k
 javascriptcoregtk4.0   x86_64   2.42.2-1.fc37       qubes-dom0-cached   8.4 M
 webkit2gtk4.0          x86_64   2.42.2-1.fc37       qubes-dom0-cached    24 M

Possibly GTK is better because there is a high chance everything is already installed.

@marmarek
Copy link
Member

It's also okay to start with zenity (which AFAIR is already pulled in by something else) for the list-only option, and later replace with something else for list+screenshots. @marmarta can help with GTK dialog.

@ben-grande ben-grande force-pushed the choose-monitor branch 3 times, most recently from d139d38 to 88d08f8 Compare June 2, 2024 10:20
@ben-grande
Copy link
Contributor Author

Zenity dialog ready for review. Sorry about many pushes, had some troubles in the rebase and merge.

@ben-grande ben-grande force-pushed the choose-monitor branch 2 times, most recently from 585a6c1 to b4bf184 Compare June 2, 2024 12:40
If more than one monitor is found on the target, an interactive dialog
will be prompted to the user to choose from, displaying monitor name
(port) and resolution and geometry for better identification. The
monitor can be enforced by the target by environment variable.

Fixes: QubesOS/qubes-issues#9275
* qubesos/main:
  Update policy format
  Export dbus address for the notification server
@ben-grande
Copy link
Contributor Author

Updated commit message.

@ben-grande
Copy link
Contributor Author

@marmarta

I have no prior experience with Gtk other than this PR, but I can grasp some examples that I've read about screenshots with pixbuf and combobox.

One design idea is taking screenshot of monitors using Gdk.pixbuf_get_from_window and setting the geometry to the monitor one. Then a combobox to select the monitor and when an option is selected, show screenshot of wanted monitor. Do not show screenshot of all monitors at the same time as many monitor can coexist and would have to reduce image resolution.

window = Gdk.get_default_root_window()
x, y, width, height = window.get_geometry()

monitor_pixbuf = Gdk.pixbuf_get_from_window(window, x, y, width, height)

if monitor_pixbuf:
    monitor_pixbuf.savev("monitor_NAME.png", "png", (), ())
else:
    ## ERROR OUT

Another option is a design similar to the KDE screen arrangement would be one option and the user click on the display or on a combobox that selects the display per name.

Copy link
Member

@marmarek marmarek left a comment

Choose a reason for hiding this comment

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

Lets merge zenity version first, and extend in a separate PR

@marmarek marmarek merged commit b58df77 into QubesOS:main Jun 3, 2024
1 of 2 checks passed
@ben-grande ben-grande deleted the choose-monitor branch June 3, 2024 07:26
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.

Qubes Video Companion screenshare sender should give option to select Dom0 monitor
2 participants