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

Caps Lock indicator colors not used #343

Open
JohnMertz opened this issue Feb 9, 2024 · 3 comments
Open

Caps Lock indicator colors not used #343

JohnMertz opened this issue Feb 9, 2024 · 3 comments

Comments

@JohnMertz
Copy link

System/Version

Version: 1.7.2
Distribution: Fedora Sericea (Sway spin of Silverblue) 39

I have not yet tested from Git, but will follow up with that when I have the time. However, the blame for the lines mentioned below shows that it has not changed since the feature was added in 1e7696f

Problem

This is a fairly trivial bug as far as the impact on the user and I suspect probably in what is needed to fix it.

The actual problem is that the config values for caps-lock-key-hl-color, caps-lock-bs-hl-color and ring-caps-lock-color don't seem to be used. When CapsLock is active, the indicator just uses the regular colors for key-hl-color, bs-hl-color and ring-color, respectively.

Steps to replicate

Have a config file with values for:

caps-lock-key-hl-color=AAAAAAFF
caps-lock-bs-hl-color=AAAAAAFF
ring-caps-lock-color=AAAAAAFF

which differ from the values for:

key-hl-color=BBBBBBFF
bs-hl-color=BBBBBBFF
ring-color=BBBBBBFF

Run swaylock type/backspace with and without CapsLock enabled. The former values will never be displayed.

Notes

At a quick glace, the conditions to select these colors look like:

swaylock/render.c

Lines 277 to 281 in b63aaff

if (state->xkb.caps_lock && state->args.show_caps_lock_indicator) {
cairo_set_source_u32(cairo, state->args.colors.caps_lock_key_highlight);
} else {
cairo_set_source_u32(cairo, state->args.colors.key_highlight);
}

I suspect that one of the two conditions to confirm the state is incorrect. An alternative might be that the correct color value is never assigned or is overwritten by the regular colors.

Note that the text-caps-lock-color value is used correctly.

I haven't looked at this beyond locating the above LOC. I can probably fix this, but won't likely have time until the weekend. Just thought that I'd report before opening a PR in case someone else can get to it quicker, correct my understanding, or point me in a better direction.

@budimanjojo
Copy link

I can confirm this also happens with https://github.com/jirutka/swaylock-effects. Maybe you can also create a PR there too (after you finished with the PR here) if you want? Or I will be glad to help with the PR in there.

@budimanjojo
Copy link

I think I know why, it's by design:

swaylock/render.c

Lines 21 to 33 in b63aaff

} else {
if (state->xkb.caps_lock && state->args.show_caps_lock_indicator) {
cairo_set_source_u32(cairo, colorset->caps_lock);
} else if (state->xkb.caps_lock && !state->args.show_caps_lock_indicator &&
state->args.show_caps_lock_text) {
uint32_t inputtextcolor = state->args.colors.text.input;
state->args.colors.text.input = state->args.colors.text.caps_lock;
cairo_set_source_u32(cairo, colorset->input);
state->args.colors.text.input = inputtextcolor;
} else {
cairo_set_source_u32(cairo, colorset->input);
}
}

Based on my understanding from the code, the caps-lock colorset is only used when --indicator-caps-lock flag is set to true. The text color of capslock is used even if the flag is not set which explains why the text color is being used.

So this is expected result based on the code, but I would suggest to not change the text color of capslock when --indicator-caps-lock flag is not true because it confuses people.

@budimanjojo
Copy link

I suggest line 24-39 in the snippet above to be removed, not sure why they're there. Is there any reason for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants