Skip to content

Add Basic XDG Support#1

Merged
tuxinal merged 5 commits into
tuxinal:mainfrom
TheMoogle:basic-xdg
Jan 13, 2025
Merged

Add Basic XDG Support#1
tuxinal merged 5 commits into
tuxinal:mainfrom
TheMoogle:basic-xdg

Conversation

@TheMoogle
Copy link
Copy Markdown
Contributor

Right now it just falls back on xcb when the portal isn't supported.
I also have allowed to pass through the required variables needed when using wayland.
This also allows the X server to use the portal path if supported.

I have not tested it with the upstream patches to vencord.

Marked as draft, as I'm still working on getting the actual portal implementation working.

Right now it just falls back on xcb when the portal isn't supported. I
also have passed through the required variables needed when using
wayland. This also allows the X server to use the portal path if
supported.
@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Sep 7, 2024

Hello there! thank you for the pull request!!
i have a couple of notes so far

i'd rather it not use the portal implementation on X11 since portal implementations might not really do what we want them to do. for example i believe the kde implementation "consumes" inputs when a shortcut is triggered, meanwhile my implementation with libuiohook doesn't do this. (also what what happens when we are on wayland and there is no implementation for GlobalShortcuts? i don't think your code handles that scenario)

it would also be nice if the window and display IDs were not required. i believe it works without them just fine
the main issue with those is i don't really know if there is a way to provide those from electron

@TheMoogle
Copy link
Copy Markdown
Contributor Author

Hello there! thank you for the pull request!! i have a couple of notes so far

i'd rather it not use the portal implementation on X11 since portal implementations might not really do what we want them to do. for example i believe the kde implementation "consumes" inputs when a shortcut is triggered, meanwhile my implementation with libuiohook doesn't do this. (also what what happens when we are on wayland and there is no implementation for GlobalShortcuts? not good things i don't think your code handles that scenario)

Would it be better to have the portal be off by default on x11, but to have an environment variable to enable it?
Right now in this pr I currently have it so that wayland falls back on the x11 capture. For most environments that should just result in only allowing shortcuts with X windows, or on KDE with the option all windows.

it would also be nice if the window and display IDs were not required. i believe it works without them just fine the main issue with those is i don't really know if there is a way to provide those from electron

I haven't personally tested it, but I'll try it on my end.

@TheMoogle
Copy link
Copy Markdown
Contributor Author

Can confirm it works without window and display ids
Screenshot_20240907_214448

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Sep 8, 2024

have an environment variable to enable it

i suppose you could. i don't see why anyone would want to use it tho

Right now in this pr I currently have it so that wayland falls back on the x11 capture. For most environments that should just result in only allowing shortcuts with X windows, or on KDE with the option all windows.

i do think that's a little weird in terms of ux. what i had planned for when we are running on wayland and there is no globalshortcuts implementation was to make a cli for triggering shortcuts in discord and make the user manually configure their DE to call it. having it only work on X11 windows in that scenario would be weird i think

@TheMoogle
Copy link
Copy Markdown
Contributor Author

have an environment variable to enable it

i suppose you could. i don't see why anyone would want to use it tho

The case you provided for not using the portal on X11 provides a KDE issue. Someone might want to use the portal on X11.

Right now in this pr I currently have it so that wayland falls back on the x11 capture. For most environments that should just result in only allowing shortcuts with X windows, or on KDE with the option all windows.

i do think that's a little weird in terms of ux. what i had planned for when we are running on wayland and there is no globalshortcuts implementation was to make a cli for triggering shortcuts in discord and make the user manually configure their DE to call it. having it only work on X11 windows in that scenario would be weird i think

That's fine, but the cli stuff is out of scope with what I'm doing with this pr. If you want me to just completely fail when the portal fails on wayland I can do that.

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Sep 9, 2024

yeah i'm gonna implement the cli stuff myself (and in fact i basically have already in vesktop)
a whole fail does seem a little dramatic. i think it would be better if there was a way to communicate it with vesktop so it can maybe inform the user. (though this could be a thing we do later. you don't have to implement it. just do an error for now)

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Sep 9, 2024

btw is everything working so far? one issue i was facing was that (on the kde portal implementation at least) it would create a new shortcut section thing every time venbind's started up. are you also facing this?
i was about to file a bug report to kde about this

@TheMoogle
Copy link
Copy Markdown
Contributor Author

TheMoogle commented Sep 9, 2024

The reason I believe it does that is the app doesn't have a desktop file when running the tests, or the window identifier not having a valid window is causing it.

@TheMoogle
Copy link
Copy Markdown
Contributor Author

I've looked into it and it is possible to get the native window handle in electron, but I haven't tested it yet.

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Sep 9, 2024

The reason I believe it does that is the app doesn't have a desktop file when running the tests, or the window identifier not having a valid window is causing it.

i'm pretty sure i have tested it with a valid window identifier before but desktop file does sound plausible

I've looked into it and it is possible to get the native window handle in electron, but I haven't tested it yet.

oh is that documented anywhere?

@TheMoogle
Copy link
Copy Markdown
Contributor Author

oh is that documented anywhere?

https://www.electronjs.org/docs/latest/api/browser-window#wingetnativewindowhandle

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Sep 9, 2024

i would like to confirm having a proper .desktop file does make it show the correct name in KCM. but it also still appends the session token to it's name and it still doesn't reuse the shortcuts from the previous session. i'm in the process of making a bug report to the kde people about this.

@TheMoogle
Copy link
Copy Markdown
Contributor Author

TheMoogle commented Sep 9, 2024

i would like to confirm having a proper .desktop file does make it show the correct name in KCM. but it also still appends the session token to it's name and it still doesn't reuse the shortcuts from the previous session. i'm in the process of making a bug report to the kde people about this.

pretty sure this is because in the protocol you have to manually restore the shortcuts, and it isn't being done yet.
https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.GlobalShortcuts.html#org-freedesktop-portal-globalshortcuts-listshortcuts

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Sep 9, 2024

i couldn't get it to work with that

@TheMoogle
Copy link
Copy Markdown
Contributor Author

it could be the window handle?

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Sep 9, 2024

i couldn't get it to work with a window handle either. window handle seemingly only gets used for aligning the settings pop-up to the correct window. it shouldn't be relevant at all.

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Jan 10, 2025

Hello! i've made a pull request for the kde portal regarding the issues we've been having. once that's merged i believe we are ready to merge. are you alright with me merging this?

@Covkie
Copy link
Copy Markdown

Covkie commented Jan 12, 2025

if the workflow for this PR is not this I dont think its ready

  • on start call ListShortcuts. If all shortcuts are registered skip BindShortcuts and take the returned trigger_description for each shortcut to display in the Discord Keybinds UI in the 'bind' box.

    • Call BindShortcuts with all possible global shortcuts a user may want (without a preferred_trigger, let users pick in their settings) also take the trigger_description response for the ui
  • Listen for the ShortcutsChanged signal and take the trigger_description for use like ListShortcuts.

  • Listen for Activated and Deactivated signals for triggering the shortcuts

  • The Discord Keybinds UI will simply function as a display of what is bound and how to trigger them in XDG mode. Maybe also have a note to users to use their System settings to configure the binds

@checkraisefold
Copy link
Copy Markdown
Contributor

if the workflow for this PR is not this I dont think its ready

* on start call ListShortcuts. If all shortcuts are registered skip BindShortcuts and take the returned `trigger_description` for each shortcut to display in the Discord Keybinds UI in the 'bind' box.
  
  * Call BindShortcuts with all possible global shortcuts a user may want (without a `preferred_trigger`, let users pick in their settings) also take the `trigger_description` response for the ui

* Listen for the ShortcutsChanged signal and take the `trigger_description` for use like ListShortcuts.

* Listen for Activated and Deactivated signals for triggering the shortcuts

* The Discord Keybinds UI will simply function as a display of what is bound and how to trigger them in XDG mode. Maybe also have a note to users to use their System settings to configure the binds

This is the final target workflow but this is mainly a merge for basic support. We'll have to overhaul registration and keybind triggers in another PR since right now keyup isn't even used, so we can refine the windows/xdg implementations once they're in

@tuxinal
Copy link
Copy Markdown
Owner

tuxinal commented Jan 13, 2025

yeah i think the same thing. i'd be happy with basic support.

Listen for Activated and Deactivated signals for triggering the shortcuts

also i believe this is handled?

i think this pr is ready enough to be merged and i'll undraft it myself

@tuxinal tuxinal marked this pull request as ready for review January 13, 2025 06:42
@tuxinal tuxinal merged commit 4156728 into tuxinal:main Jan 13, 2025
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.

4 participants