Skip to content
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

Improve fallback behavior for unset device config values #3184

Merged
merged 4 commits into from
Sep 6, 2023

Conversation

pdamianik
Copy link
Contributor

@pdamianik pdamianik commented Sep 5, 2023

Describe your PR, what does it fix/add?

This should improve the fallback process for device specific, unset config values. It replaces the touchpad bool parameter for getDevice... functions with an optional fallback configuration key that describes the global configuration key to fall back to if the device specific key is unset. The previous implementation didn't quite work for me, as it would reset natural_scroll to disabled even when just enabling/disabling my touchpad. This should also handle conflicts between the fallback configs input:touchdevice:transform and input:tablet:transform and input:touchdevice:output and input:tablet:output better as the current implementation has a hardcoded order which prefers touchdevice:* keys over tablet:* keys, even for tablet devices.

Alternative solution

An alternative solution would be to use the same key structure as the normal keys for device config keys. This could improve intuitiveness for users (the keys aren't just the last section and potentially conflicting) and it would make the extra fallback argument unnecessary.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

CConfigManager::getConfigValueSafeDevice is overloaded and spreads to CConfigManager::getDevice..., so I originally tried to use std::optional (I have very little C++ experience). Is there a better way to describe an optional string reference?

Is it ready for merging, or does it need work?

If it looks good and the alternative solution is not something to be implemented, it's ready 😃 .

@pdamianik pdamianik changed the title Fix fallback behavior for unset device config values Improve fallback behavior for unset device config values Sep 5, 2023
@pdamianik pdamianik force-pushed the improve-device-config-fallback branch from db5db2e to a9a3e51 Compare September 6, 2023 08:29
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

oh, and conflict

src/config/ConfigManager.cpp Outdated Show resolved Hide resolved
src/config/ConfigManager.hpp Outdated Show resolved Hide resolved
src/config/ConfigManager.hpp Outdated Show resolved Hide resolved
@pdamianik pdamianik force-pushed the improve-device-config-fallback branch from a9a3e51 to e36efe0 Compare September 6, 2023 12:30
@pdamianik
Copy link
Contributor Author

pdamianik commented Sep 6, 2023

Fixed, what do you think about using the same key structure for device configs as the global config?

@vaxerski
Copy link
Member

vaxerski commented Sep 6, 2023

not sure what you mean by that

@pdamianik
Copy link
Contributor Author

Nevermind, pr is ready from my side.

@vaxerski
Copy link
Member

vaxerski commented Sep 6, 2023

I don't think this will compile xD

@pdamianik
Copy link
Contributor Author

pdamianik commented Sep 6, 2023

It did (hold on) on my machine :), but we'll see

@vaxerski
Copy link
Member

vaxerski commented Sep 6, 2023

also #3189 got merged in the meantime so if you could rebase against master and fix that up

@vaxerski
Copy link
Member

vaxerski commented Sep 6, 2023

told you it wont compile :P

header:
const std::string& param = ""
source:
const std::string& param

you can't have = "" in the source

@pdamianik pdamianik force-pushed the improve-device-config-fallback branch from e36efe0 to e5b6ecd Compare September 6, 2023 13:19
@pdamianik
Copy link
Contributor Author

Yeah, that's what happens when you do your first C++ coding.

@pdamianik pdamianik force-pushed the improve-device-config-fallback branch from e5b6ecd to 32a3838 Compare September 6, 2023 13:21
@pdamianik
Copy link
Contributor Author

pdamianik commented Sep 6, 2023

Fixed, compiles, starts (tested for the latest changes this time), fixes my touchpad issue, rebased and ready for merging.

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

some typos.

As a next step (for the future) since we have fallbacks, we could drop the HASCONFIG and move it to CConfigManager::getConfigValueSafeDevice

src/managers/input/InputManager.cpp Outdated Show resolved Hide resolved
src/managers/input/InputManager.cpp Outdated Show resolved Hide resolved
src/config/ConfigManager.cpp Outdated Show resolved Hide resolved
@pdamianik
Copy link
Contributor Author

some typos.

As a next step (for the future) since we have fallbacks, we could drop the HASCONFIG and move it to CConfigManager::getConfigValueSafeDevice

I'll make another branch ready.

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

h

@vaxerski
Copy link
Member

vaxerski commented Sep 6, 2023

ya

@vaxerski vaxerski merged commit a15e3e1 into hyprwm:main Sep 6, 2023
8 checks passed
@pdamianik pdamianik deleted the improve-device-config-fallback branch September 6, 2023 14:19
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.

2 participants