Skip to content

Conversation

@gujie-leica
Copy link

@gujie-leica gujie-leica commented May 20, 2025

Hello fellow geeks,

I add key navigation inside of notification:

  • left, right, up and down arrow keys select the button (action)
  • enter key executes action
  • add “focus“ when selected, the first key is the default focus key

I tested it in Wayland, hope this makes the built-in menu more user-friendly.

This PR is based on #1436, which introduces a built-in clickable menu feature.

Additionally, since this functionality depends on libxkbcommon, I have added it to the CI Docker image in dunst-project/docker-images#16.

@gujie-leica gujie-leica force-pushed the add-navigation-for-clickable-menu branch 5 times, most recently from 7ae6447 to fec41e4 Compare May 20, 2025 09:53
@gujie-leica gujie-leica marked this pull request as ready for review May 20, 2025 10:00
gujie-leica added a commit to gujie-leica/docker-images that referenced this pull request May 21, 2025
Since we need to use keyboard process in "Add navigation for clickable menu"
(dunst-project/dunst#1480), add libxkbcommon.

Signed-off-by: gujie <[email protected]>
Add menu related function to prepare key navigation feature.

Signed-off-by: gujie <[email protected]>
Introduce built-in menu keybaord settings in dunstrc.

Signed-off-by: gujie <[email protected]>
@gujie-leica gujie-leica force-pushed the add-navigation-for-clickable-menu branch from fec41e4 to 079185e Compare August 1, 2025 03:29
@gujie-leica
Copy link
Author

V2: rebase code.

@bynect
Copy link
Contributor

bynect commented Aug 4, 2025

i added libxkbc to the dockerfiles. ci looks good, the only problem is something with doxygen

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 4.37500% with 306 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.83%. Comparing base (068e4e9) to head (079185e).
⚠️ Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
src/draw.c 12.61% 97 Missing ⚠️
src/menu.c 0.00% 95 Missing ⚠️
src/wayland/wl_seat.c 0.00% 62 Missing ⚠️
src/input.c 0.00% 48 Missing ⚠️
src/queues.c 0.00% 4 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1480      +/-   ##
==========================================
- Coverage   64.90%   62.83%   -2.07%     
==========================================
  Files          51       51              
  Lines        9024     9337     +313     
  Branches     1048     1128      +80     
==========================================
+ Hits         5857     5867      +10     
- Misses       3167     3470     +303     
Flag Coverage Δ
unittests 62.83% <4.37%> (-2.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@bynect bynect left a comment

Choose a reason for hiding this comment

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

i didn't write a lot because most of the code is for the menu. I will also review the other pr

ifneq (0,${WAYLAND})
pkg_config_packs += wayland-client
pkg_config_packs += wayland-cursor
pkg_config_packs += xkbcommon
Copy link
Contributor

Choose a reason for hiding this comment

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

is xkb only used in wayland?

Copy link
Author

Choose a reason for hiding this comment

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

is xkb only used in wayland?

In the current code, libxkbcommon is used only for Wayland, as X11 has its own built-in XKB extension and doesn't require libxkbcommon.

} touch;

struct {
struct wl_keyboard *wl_keyboard;
Copy link
Contributor

Choose a reason for hiding this comment

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

if you know this well, do you think this wayland keyboard interface can also be used for the shortcuts that dunst offers in Xorg? (https://dunst-project.org/documentation/#Keyboard-shortcuts-X11-only)

Copy link
Author

Choose a reason for hiding this comment

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

if you know this well, do you think this wayland keyboard interface can also be used for the shortcuts that dunst offers in Xorg? (https://dunst-project.org/documentation/#Keyboard-shortcuts-X11-only)

In my opinion, menu navigation utilizes simple arrow keys and 'Enter' within the notification context, which is fundamentally different from global shortcuts that require modifier combinations, such as 'modifier' +Key. We provide intuitive UI navigation similar to dialog boxes or web forms, while the existing shortcuts are system-wide hotkeys for operations like closing all notifications or viewing history.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bynect This is only the standard keyboard interface for wayland applications to get keyboard events. Usually the compositor will only pass the keyboard events to the currently focused application. Given that dunst doesn't want (and probably can't) steal the focus, it'll only work for a limited set of use cases.

For proper global shortcuts the direction seems to be the Global Shortcuts Portal API from Flatpack. GNOME already implements it. But for wl-roots it's still an open issue: emersion/xdg-desktop-portal-wlr#240

Copy link
Contributor

Choose a reason for hiding this comment

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

that's what I thought. thank you for confirmation 👍🏻

Add function to handle built-in menu key navigation.

Signed-off-by: gujie <[email protected]>
The design now utilizes an inverted display when the key is in
focus, with the first key being in focus when the menu appears.

Signed-off-by: gujie <[email protected]>
@gujie-leica gujie-leica force-pushed the add-navigation-for-clickable-menu branch from 079185e to 069cba5 Compare August 8, 2025 06:37
@fwsmit
Copy link
Member

fwsmit commented Oct 7, 2025

This PR is based on #1436, containing a lot of the same code. However it uses totally different commits. This means that if #1436 is merged, this PR would likely have a lot of merge conflicts. I suggest either dropping #1436, because this all of its functionality, or properly rebasing on this PR (which would involve some git trickery).

@bynect
Copy link
Contributor

bynect commented Oct 8, 2025

I originally proposed to split the menu button and this pr since this added libxkd dependency. I think the best course is to merge the other pr first and then rebase this one on top of it.

@fwsmit
Copy link
Member

fwsmit commented Oct 9, 2025

I originally proposed to split the menu button and this pr since this added libxkd dependency. I think the best course is to merge the other pr first and then rebase this one on top of it.

Yeah, that could work

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.

5 participants