-
Notifications
You must be signed in to change notification settings - Fork 177
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
Input device updates #2510
base: master
Are you sure you want to change the base?
Input device updates #2510
Conversation
soreau
commented
Nov 6, 2024
•
edited
Loading
edited
- Adds the ability to configure input devices that might have matching names
- Refactor so hotplugging a device only refreshes the configuration for that device, not all devices
- Add calibration option for touch devices
- Add a way to configure multiple keyboards with different settings
8f989e9
to
91db71d
Compare
This patch adds additional checks for input-device sections in the config file. It checks for ID_PATH, ID_SERIAL, LIBINPUT_DEVICE_GROUP then falls back to the device name and finally the global input-device section. In addition, DEVPATH is printed to log when WF_PRINT_UDEV_DEVPATH is set and '-d' is passed to wayfire, for matching the correct device in i.e. 'udevadm info --tree'. The sections that wayfire checks for are also logged with '-d'. This makes it so users can match devices, especially identical devices lacking many of the usual differentiating properties. Fixes #2504.
Before, hotplugging a new device would refresh all input to output mappings. Hotplugging an output will still refresh all input device mappings.
This adds a calibration option so that touch input devices can be calibrated via wayfire configuration.
This patch checks for additional configuration sections so keyboards can be matched by name or with udev properties. Running wayfire with '-d' will log the sections that are being checked, for reference. This allows multiple keyboards to be configured differently, whereas before, all keyboards used the common configuration in the [input] section. To be clear, no additional configuration is required. If no matching configuration section is found for the device, [input] options will be used. Fixes #2481.
91db71d
to
fcc81c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+10 for the idea. I was thinking of whether we can make this work fully dynamically (currently if you want to add a section, you have to restart Wayfire) but I don't think this can be done easily.
std::shared_ptr<config::section_t> wf::config_backend_t::get_input_device_section( | ||
wlr_input_device *device) | ||
{ | ||
auto& config = wf::get_core().config; | ||
std::shared_ptr<wf::config::section_t> section; | ||
auto print_devpath = getenv("WF_PRINT_UDEV_DEVPATH"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you are looking for here are the debug categories, they are rather simple to add, maybe an input-device
category?
Check these here: https://github.com/WayfireWM/wayfire/blob/master/src/api/wayfire/debug.hpp#L42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, this makes more sense. This way we can use wayfire -d input-device
to get the additional message(s).
{ | ||
for (struct udev_property_and_desc const & pd : properties_and_descs) | ||
{ | ||
if (!print_devpath && !strncmp(pd.property_name, "DEVPATH", strlen("DEVPATH"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, wouldn't it be more appropriate to send this info over via IPC and print it in the scripts we already have for listing input devices? I can imagine that this would be much easier to use.
Otherwise, I think we can/should just print every property we are querying here (in the corresponding debug category).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree that we should require ipc for this task. It should be as easy as wayfire -d input-device
(or plural, input-devices).
* Calibrate a touch device with a matrix. This function does nothing | ||
* if called with a device that is not a touch device. | ||
*/ | ||
void calibrate_touch_device(wlr_input_device *dev, std::string const & cal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like it should be in input-manager ... Because this is device-type-specific (mapping inputs to outputs works for tablets, touch and pointer). Ideally speaking, we'd want to add an input-device class for touch devices and load their specific options there, just like we do for pointers: https://github.com/WayfireWM/wayfire/blob/master/src/core/seat/pointing-device.hpp
And then we can create this device-specific object here:
wayfire/src/core/seat/input-manager.cpp
Line 39 in 5653242
At the very least, it seems like this should be somewhere in the input device class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, ok..
@@ -49,7 +49,7 @@ class config_backend_t | |||
* described in input-device.xml | |||
*/ | |||
virtual std::shared_ptr<config::section_t> get_input_device_section( | |||
wlr_input_device *device); | |||
std::string const & prefix, wlr_input_device *device); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs API/ABI version bump :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I forgot to bump! :)
|
||
repeat_rate.load_option("input/kb_repeat_rate"); | ||
repeat_delay.load_option("input/kb_repeat_delay"); | ||
auto section = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we'd want the same for pointer?
wayfire/src/core/seat/pointing-device.cpp
Lines 12 to 31 in 5653242
left_handed_mode.load_option("input/left_handed_mode"); | |
middle_emulation.load_option("input/middle_emulation"); | |
mouse_scroll_speed.load_option("input/mouse_scroll_speed"); | |
mouse_cursor_speed.load_option("input/mouse_cursor_speed"); | |
touchpad_cursor_speed.load_option("input/touchpad_cursor_speed"); | |
touchpad_scroll_speed.load_option("input/touchpad_scroll_speed"); | |
mouse_natural_scroll_enabled.load_option("input/mouse_natural_scroll"); | |
touchpad_tap_enabled.load_option("input/tap_to_click"); | |
touchpad_dwt_enabled.load_option("input/disable_touchpad_while_typing"); | |
touchpad_dwmouse_enabled.load_option("input/disable_touchpad_while_mouse"); | |
touchpad_natural_scroll_enabled.load_option("input/natural_scroll"); | |
touchpad_drag_lock_enabled.load_option("input/drag_lock"); | |
mouse_accel_profile.load_option("input/mouse_accel_profile"); | |
touchpad_accel_profile.load_option("input/touchpad_accel_profile"); | |
touchpad_click_method.load_option("input/click_method"); | |
touchpad_scroll_method.load_option("input/scroll_method"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking what other bits we might want this for.. adding for pointer makes sense to me, will update.
Thanks for the review! However, I don't think it is true that you need to restart wayfire, but only hot(re)plug the input device in question. Of course this might not be completely optimal, but a full restart is not needed as far as I understand. |