Make key repeat and delay configurable #3730
Conversation
tarek-y-ismail
force-pushed
the
MIRENG-892/configurable-key-repeat
branch
from
4712763 to
9299fb6
Compare
tarek-y-ismail
force-pushed
the
MIRENG-892/configurable-key-repeat
branch
from
9299fb6 to
a6b1793
Compare
tarek-y-ismail
changed the base branch from
main
to
config-file-load-after-server-init
tarek-y-ismail
force-pushed
the
MIRENG-892/configurable-key-repeat
branch
from
a6b1793 to
55c6cb7
Compare
Shells can now set the repeat rate and delay based on their config file format.
tarek-y-ismail
force-pushed
the
MIRENG-892/configurable-key-repeat
branch
from
55c6cb7 to
384afb6
Compare
tarek-y-ismail
force-pushed
the
config-file-load-after-server-init
branch
2 times, most recently
from
f6f04ce to
6a0f1b9
Compare
|
Note to self: Just came across this while working on something else, might be a good idea to check out mir/src/server/input/default_configuration.cpp Lines 144 to 164 in 5e15fde |
Co-authored-by: Matthew Kosarek <[email protected]>
tarek-y-ismail
force-pushed
the
MIRENG-892/configurable-key-repeat
branch
from
f66af7f to
d484803
Compare
Very confused what this does since repeat on both wayland and X seem to work without any modification to it. Edit: From a quick skim, this seems to implement key repeat on the server. For wayland and X(wayland), the code changes here suffice. |
Initially, the config file is loaded before the server starts. The changes need some server components to be applied, so they aren't applied at that stage. The few lines added store the changes and apply them once the server starts.
tarek-y-ismail
changed the base branch from
config-file-load-after-server-init
to
main
| miral::InputConfiguration input_configuration; | ||
| auto mouse = input_configuration.mouse(); | ||
| auto touchpad = input_configuration.touchpad(); | ||
| auto keyboard = input_configuration.keyboard(); |
There was a problem hiding this comment.
I think we could grow a utility class that combines these and the repeated "apply" logic?
Also, we need to be careful: we're now accessing mouse, touchpad and keyboard on multiple threads.
There was a problem hiding this comment.
Having said that, I like the revised approach
There was a problem hiding this comment.
Hmm, do you have anything particular in mind? The simplest solution I can think of is keeping the initialization logic as is and wrapping the application logic in a lambda with a mutex.
Something like:
+ std::mutex config_mutex;
+
+ auto config_applier = [&] mutable
+ {
+ input_configuration.mouse(mouse);
+ input_configuration.touchpad(touchpad);
+ input_configuration.keyboard(keyboard);
+ };
miral::ConfigFile test{runner, "mir_demo_server.input", miral::ConfigFile::Mode::reload_on_change,
[&](auto& in, auto path)
{
std::cout << "** Reloading: " << path << std::endl;
+ std::lock_guard lock{config_mutex};
for (std::string line; std::getline(in, line);)
{
@@ -185,17 +196,14 @@ try
}
}
- input_configuration.mouse(mouse);
- input_configuration.touchpad(touchpad);
- input_configuration.keyboard(keyboard);
+ config_applier();
}};
runner.add_start_callback(
[&]
{
- input_configuration.mouse(mouse);
- input_configuration.touchpad(touchpad);
- input_configuration.keyboard(keyboard);
+ std::lock_guard lock{config_mutex};
+ config_applier();
But I don't really think this needs its own utility class, just having something like this as an example should be enough.
Edit: Moved the lock to be external to config_applier since we also need to protect mouse, keyboard, and touchpad from being modified as we apply the config or vice-versa
There was a problem hiding this comment.
It is always possible to have data and related functions separately, but it is clearer to the reader when they are grouped as a class. In particular, it is easier to check invariants when it is clear which code can access the data.
In this case, I think the logic around config_mutex, mouse, touchpad and keyboard is complex enough to make isolating this data desirable:
class InputConfigFile : miral::ConfigFile
{
public:
InputConfigFile(miral::MirRunner& runner, std::filesystem::path file) :
ConfigFile{runner, file, ConfigFile::Mode::reload_on_change, [this](auto args...) {loader(args...); }}
{
runner.add_start_callback(
[&]
{
std::lock_guard lock{config_mutex};
config_applier();
});
}
void operator()(mir::Server& server)
{
input_configuration(server);
}
private:
miral::InputConfiguration input_configuration;
auto mouse = input_configuration.mouse();
auto touchpad = input_configuration.touchpad();
auto keyboard = input_configuration.keyboard();
std::mutex config_mutex;
void config_applier() const
{
input_configuration.mouse(mouse);
input_configuration.touchpad(touchpad);
input_configuration.keyboard(keyboard);
};
void loader(std::istream& istream, std::filesystem::path const& path)
{
std::lock_guard lock{config_mutex};
// ...
}
};It is quick to see that the data is only touched by this code, and that it is appropriately locked.
There was a problem hiding this comment.
Here's a version without the undefined behaviour and compilation errors:
class InputConfigFile
{
public:
InputConfigFile(miral::MirRunner& runner, std::filesystem::path file) :
config_file{
runner,
file,
miral::ConfigFile::Mode::reload_on_change,
[this](std::istream& istream, std::filesystem::path const& path) {loader(istream, path); }}
{
runner.add_start_callback([this]
{
std::lock_guard lock{config_mutex};
apply_config();
});
}
void operator()(mir::Server& server)
{
input_configuration(server);
}
private:
miral::InputConfiguration input_configuration;
miral::InputConfiguration::Mouse mouse = input_configuration.mouse();
miral::InputConfiguration::Touchpad touchpad = input_configuration.touchpad();
miral::InputConfiguration::Keyboard keyboard = input_configuration.keyboard();
miral::ConfigFile config_file;
std::mutex config_mutex;
void apply_config()
{
input_configuration.mouse(mouse);
input_configuration.touchpad(touchpad);
input_configuration.keyboard(keyboard);
};
void loader(std::istream& in, std::filesystem::path const& path)
{
std::cout << "** Reloading: " << path << std::endl;
std::lock_guard lock{config_mutex};
for (std::string line; std::getline(in, line);)
{
std::cout << line << std::endl;
if (line == "mir_pointer_handedness_right")
mouse.handedness(mir_pointer_handedness_right);
if (line == "mir_pointer_handedness_left")
mouse.handedness(mir_pointer_handedness_left);
if (line == "mir_touchpad_scroll_mode_none")
touchpad.scroll_mode(mir_touchpad_scroll_mode_none);
if (line == "mir_touchpad_scroll_mode_two_finger_scroll")
touchpad.scroll_mode(mir_touchpad_scroll_mode_two_finger_scroll);
if (line == "mir_touchpad_scroll_mode_edge_scroll")
touchpad.scroll_mode(mir_touchpad_scroll_mode_edge_scroll);
if (line == "mir_touchpad_scroll_mode_button_down_scroll")
touchpad.scroll_mode(mir_touchpad_scroll_mode_button_down_scroll);
if (line.contains("="))
{
auto const eq = line.find_first_of("=");
auto const key = line.substr(0, eq);
auto const value = line.substr(eq+1);
auto const parse_and_validate = [](std::string const& key, std::string_view val) -> std::optional<int>
{
auto const int_val = std::atoi(val.data());
if (int_val < 0)
{
mir::log_warning(
"Config value %s does not support negative values. Ignoring the supplied value (%d)...",
key.c_str(), int_val);
return std::nullopt;
}
return int_val;
};
if (key == "repeat_rate")
{
auto const parsed = parse_and_validate(key, value);
if (parsed)
keyboard.set_repeat_rate(*parsed);
}
if (key == "repeat_delay")
{
auto const parsed = parse_and_validate(key, value);
if (parsed)
keyboard.set_repeat_delay(*parsed);
}
}
}
apply_config();
}
};One small annoyance is that this isn't Copyable, and that means the run_with() parameter needs passing with std::ref(). Hoisting the state into a shared Self would work, but seems overkill.
Reduces unnecessary duplication and protects the code with a mutex.
There was a problem hiding this comment.
I believe I found a bug, but I don't know where it could be in the code! Here's a reproducer for you:
- Start
mir_demo_server - Open up
gedit - Have the following input config:
repeat_rate=1 repeat_delay=1 - Hold down the 'a' key and start inserting 'a's. It should insert two 'a's almost immediately, and then 1 'a' every second or so
- Clear the gedit file. Set
repeat_rateto 2 and save the config. Then set it to 1 again immediately and save the config. - Go back to gedit and hold down the 'a' key. Two 'a's will be inserted almost immediately and then 1 'a' will appear every second or so. HOWEVER, note that at some point later, you will get the "double 'a' insertion" again, even though you didn't start inserting again!
Other than this bug, it works well for me!
From the description the bug could lie in GTK (implementing the repeat is the client's responsibility). The thing to check is what repeat_info events are being sent to the client when the edit occurs. |
Same issue with
Here was my WAYLAND_DEBUG trace when it happened. This might actually be unrelated to this work. I am definitely holding down the key on my keyboard, but its registering a release + press again. Perhaps it is just my keyboard though since it only happens some times. Can either @AlanGriffiths or @tarek-y-ismail confirm that this doesn't happen for you? |
It took a while (first few tries I didn't see it) but I finally managed to occasionally. But it doesn't look like this PR is responsible. Here's my test scenario:
Expect: "a" followed quickly (1ms later) by "a" and another "a" each second until key released Here's what I see when the "aa" happens: That is, we're telling the client the key is released and pressed at Regardless, so far as this PR is concerned, we are sending the expected |
Some "printf() debugging" supports this: That is, X11 is telling Mir the key has been released and pressed and we pass that on. (I thought we had logic to suppress these release/press pairs, but I don't see it know) |
That disappeared with the move to xcb (5207ca9) |
|
@tarek-y-ismail I think the only thing to address is my InputConfigFile suggestion. (If nobody else thinks it worthwhile, then 🤷) |
AlanGriffiths
dismissed
mattkae’s stale review
The queried behaviour isn't related to the PR
Likely the same cause as #1471 |
Great. I marked it as |
Is anyone really going to game on the X11 platform?! |
That's the only way for WINE/Proton-based games (i.e. the majority) still... |
The |
How to test:
~/.config/mir_demo_server.inputwith the following content:kgx), or and X application (xterm)repeat_rateto a higher or lower value and watch as the repeat rate speeds or slows downrepeat_delayto a higher or lower value and watch as the duration you need to hold the key down increases or decreases.