-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Added submap universal keybind flag #12100
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
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM.
Tests and wiki MR needed
|
About the {} removal, wouldn't that just be better as a switch? |
|
Yes think that would be a good idea will change it |
|
|
||
| // keybind works on default submap | ||
| OK(getFromSocket("/dispatch plugin:test:keybind 1,7,29")); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(50)); |
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.
don't sleep in tests. Runner VMs are very slow and unpredictable. Just don't rely on timings.
I don't think this sleep (and the one further down) is even needed?
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 did try removing the sleeping and the test failed, seemed like key press wasn't being registered because it was too fast but works fine not sleeping on the other key presses which is weird.
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.
Since it's touching a file and you check if the file exists, this is to be expected.
The socket call is async and so is the exec so without the sleep hyprtester just moves along and checks if the file exists before the file have been created.
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.
yeah, check for the file in a loop every x ms like 30 times
| EXPECT_CONTAINS(getFromSocket("/submap"), "submap1"); | ||
| OK(getFromSocket("/dispatch plugin:test:keybind 1,7,29")); | ||
| OK(getFromSocket("/dispatch plugin:test:keybind 0,7,29")); | ||
| EXPECT(waitCheckFlag(30, 5), true); |
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.
you dont clear the flag beforehand, is that intended?
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.
The checkFlag() function already clears it though
|
Just updated commit to change the name of the function I added |
Describe your PR, what does it fix/add?
Implements #11818. Added bind flag (u) that makes the bind ignore the submap so it is active no matter the current submap.
Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)
I updated syntax in the bind argument loop to remove {} as many cases are one liners, just checking that this is the correct style.
https://github.com/Brumus14/Hyprland/blob/71052a3850eebfae67275c60828a5efdfbde2d01/src/config/ConfigManager.cpp#L2568-L2601
Is it ready for merging, or does it need work?
Should be ready for merging.