fairyglade / ly

display manager with console UI
Do What The F*ck You Want To Public License
5.28k stars 307 forks source link

[Bug] The caps lock indicator only gets updated when you type #670

Open Blayung opened 1 month ago

Blayung commented 1 month ago

Pre-requisites

Ly version

1.0.1

Observed behavior

The caps lock indicator only gets updated when you type.

Expected behavior

The caps lock indicator updates the same moment that you press the caps lock key.

Steps to reproduce

  1. Turn ly on
  2. Press caps lock
  3. It doesn't update until you start typing

Relevant logs

No response

AnErrupTion commented 1 month ago

This error was already reported over at #607 but for numlock. Granted, the title doesn't indicate much :D

Blayung commented 1 month ago

So should this issue stay?

AnErrupTion commented 1 month ago

So should this issue stay?

You decide. It's technically different, since it's about capslock and not numlock. 👀

Blayung commented 1 month ago

Yeah, this looks way better than #607!

Like, where does he even mention numlock?

  1. Custom colors are not applied to the login box.
  2. After pressing the restart button, ly behaves as if it is stuck...it is obviously better to exit ly like the old version.
  3. The compile time prompts are not very good.

Either ways, you should fix it too.

AnErrupTion commented 1 month ago

They mention it right there: https://github.com/fairyglade/ly/issues/607#issuecomment-2254859271

llc0930 commented 1 month ago

Yeah, this looks way better than #607!

Like, where does he even mention numlock?

Sorry, maybe I should open a new issue at that time.

AnErrupTion commented 1 month ago

Yeah, this looks way better than #607! Like, where does he even mention numlock?

Sorry, maybe I should open a new issue at that time.

No worries, you can just tell your issue here. You can close your other issue if you want, though.

llc0930 commented 1 month ago

To be honest, it seems that you have already completed the repair, but I have no plans to restart recently so I can't test it. I usually restart after updating the kernel or important components. Just close it when you think it's done and we'll raise it again if there are still questions.

AnErrupTion commented 1 month ago

I only fixed it for the clocks, but not for the lock states. That's a bit more complicated, because what I think happens is that termbox is polling for TTY events (through tb_poll_event() or tb_peek_event()) but the TTY doesn't report numlock & capslock as key events. This means that the only way to fix this problem is to make a separate thread that continuously checks for the lock state, and updates the UI accordingly. I haven't tried doing that yet though, since Ly, even with the Zig rewrite, wasn't really designed for being thread-safe.

Blayung commented 1 month ago

I think I can't reproduce it anymore, but if you say that it's still broken then whatever.

llc0930 commented 1 month ago

I think I can't reproduce it anymore, but if you say that it's still broken then whatever.

That's because in 598fa6a, we temporarily reverted to the same behavior as the old version: updating the lock indicator periodically. This is actually an acceptable solution if the TTY does not report these statuses. After all, when the polling interval of the main thread is short enough, the extra checking thread seems to cause unnecessary overhead, and people are obviously not sensitive enough to need to get that feedback immediately.

Blayung commented 1 month ago

But I didn't update.

llc0930 commented 1 month ago

But I didn't update.

So mysterious.

AnErrupTion commented 1 month ago

I think I can't reproduce it anymore, but if you say that it's still broken then whatever.

That's because in 598fa6a, we temporarily reverted to the same behavior as the old version: updating the lock indicator periodically. This is actually an acceptable solution if the TTY does not report these statuses. After all, when the polling interval of the main thread is short enough, the extra checking thread seems to cause unnecessary overhead, and people are obviously not sensitive enough to need to get that feedback immediately.

That commit shouldn't have had to exist, it always worked this way. I commented that code originally because I was doing tests in my terminal without appropriate permissions to access /dev/console. With that said, if you have no animations & no clock, the lock state won't actually update until you press a key.

llc0930 commented 1 month ago

Well, I only confirmed that the original rewritten version had this code snippet. I didn't carefully check which commit it was commented. I only noticed that it was uncommented in that commit. I actually don't quite remember the lock indicator behavior from older versions, since I usually just use the core section of a keyboard to enter passwords, via muscle memory, and don't pay attention to the indicator. By the way, I just saw #161.

AnErrupTion commented 1 month ago

By the way, I just saw https://github.com/fairyglade/ly/issues/161.

I forgot about that issue. It's about capslock not being shown at all. I'll have to check if it was a feature back then, and if it wasn't, close the issue since it's been implemented since.

llc0930 commented 1 month ago

It should be before #78.

llc0930 commented 1 month ago

This feature has existed at least since ce745bf.

llc0930 commented 1 month ago

I took another look at main.zig, so it was because the drawing of all components was tied together when rewriting. In older versions, the action of the drawing lock indicator was performed at the top level of the loop's main function.

llc0930 commented 1 month ago

But I didn't update.

So maybe you have animations or a clock turned on?

AnErrupTion commented 1 month ago

I took another look at main.zig, so it was because the drawing of all components was tied together when rewriting. In older versions, the action of the drawing lock indicator was performed at the top level of the loop's main function.

That makes sense. It was basically updated independently, but still in the main render loop.

AnErrupTion commented 1 month ago

I took another look at main.zig, so it was because the drawing of all components was tied together when rewriting. In older versions, the action of the drawing lock indicator was performed at the top level of the loop's main function.

That makes sense. It was basically updated independently, but still in the main render loop.

Actually, I took a better look at the code from the commit you sent but I don't really see how it's much different from what we have right now. In Ly v0.1.0, the screen only got updated if it got forced to or if there was an event. But, we can't just make it force update every time, and as I already stated before, the TTY doesn't send any event for numlock or capslock activation.

Blayung commented 1 month ago

Aaah, yeah, it started working because just as I was creating this issue, I have changed the config to use the doom animation. Mystery solved!

llc0930 commented 1 month ago

Actually, I took a better look at the code from the commit you sent but I don't really see how it's much different from what we have right now. In Ly v0.1.0, the screen only got updated if it got forced to or if there was an event. But, we can't just make it force update every time, and as I already stated before, the TTY doesn't send any event for numlock or capslock activation.

Yes, I didn't look carefully yesterday. I later remembered that I didn't even realize ly had a lock indicator until I enabled the then-new clock feature(#461). I raised #522 after enabling the clock because I first saw the indicator appear on the screen.

llc0930 commented 1 month ago

Hope these can help you:

AnErrupTion commented 1 month ago

Hope these can help you:

* [event-interface](https://www.kernel.org/doc/html/latest/input/input.html#event-interface), [event-codes](https://www.kernel.org/doc/html/latest/input/event-codes.html)
  `evtest` output:
  ```
  Event: time 1722635448.728630, type 17 (EV_LED), code 0 (LED_NUML), value 1
  Event: time 1722635448.728630, type 17 (EV_LED), code 1 (LED_CAPSL), value 1
  ```

  The reason why the `EV_KEY` event is not directly monitored is because the user may exchange key codes for use. For example, I swapped the uses of `KEY_LEFTCTRL` and `KEY_CAPSLOCK`.

* [Linux](https://github.com/torvalds/linux/blob/master/include/uapi/linux/input-event-codes.h)

* [FreeBSD](https://cgit.freebsd.org/src/plain/sys/dev/evdev/input-event-codes.h)

* [DragonFlyBSD](https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/dev/misc/evdev/input-event-codes.h)

* OpenBSD doesn't seem to want to port evdev?
  https://github.com/openbsd/src/blob/58e8bf10994e0b5fb0845d9b128c6346ec2a795e/sys/dev/hid/hid.h#L400-L402
  https://github.com/openbsd/src/blob/58e8bf10994e0b5fb0845d9b128c6346ec2a795e/sys/dev/hid/hidkbd.c#L794-L797

Hope these can help you:

* [event-interface](https://www.kernel.org/doc/html/latest/input/input.html#event-interface), [event-codes](https://www.kernel.org/doc/html/latest/input/event-codes.html)
  `evtest` output:
  ```
  Event: time 1722635448.728630, type 17 (EV_LED), code 0 (LED_NUML), value 1
  Event: time 1722635448.728630, type 17 (EV_LED), code 1 (LED_CAPSL), value 1
  ```

  The reason why the `EV_KEY` event is not directly monitored is because the user may exchange key codes for use. For example, I swapped the uses of `KEY_LEFTCTRL` and `KEY_CAPSLOCK`.

* [Linux](https://github.com/torvalds/linux/blob/master/include/uapi/linux/input-event-codes.h)

* [FreeBSD](https://cgit.freebsd.org/src/plain/sys/dev/evdev/input-event-codes.h)

* [DragonFlyBSD](https://github.com/DragonFlyBSD/DragonFlyBSD/blob/master/sys/dev/misc/evdev/input-event-codes.h)

* OpenBSD doesn't seem to want to port evdev?
  https://github.com/openbsd/src/blob/58e8bf10994e0b5fb0845d9b128c6346ec2a795e/sys/dev/hid/hid.h#L400-L402
  https://github.com/openbsd/src/blob/58e8bf10994e0b5fb0845d9b128c6346ec2a795e/sys/dev/hid/hidkbd.c#L794-L797

The problem is that we'd still be limited to termbox2's event peeking functions. Since the way they work doesn't allow for retrieving the numlock & capslock key states, the only way to fix this issue would be to stop using those entirely and use the event interface mentioned above. However, I'm afraid this may hinder portability & porting efforts, like with the mentioned OpenBSD example.

Also, while we only do handle key events, there are other types of events, possibly like resolution changes (though I'm not quite sure if those are part of TTY events).

llc0930 commented 1 month ago

I guess it depends on what these indicators are trying to achieve.

If it is just to make the user notice after typing that what they have just entered is a capital letter or number, then it has actually succeeded. But that's obviously not what it should be.

Maybe people do want to know immediately that Caps Lock or Num Lock is enabled. Well, as you say, making these low-level changes may hinder portability & porting efforts.

However, do people really need to receive immediate feedback from these indicators? I think you could consider using tb_peek_event() directly with 100ms, humans are not that sensitive.

if (animate and !animation_timed_out) {
    timeout = config.min_refresh_delta;
    ...
} else {
    var tv: interop.system_time.timeval = undefined;
    _ = interop.system_time.gettimeofday(&tv, null);

    timeout = @intCast(100 - @rem(@divTrunc(tv.tv_usec, 1000), 100));
}
const event_error = termbox.tb_peek_event(&event, timeout);
AnErrupTion commented 1 month ago

However, do people really need to receive immediate feedback from these indicators? I think you could consider using tb_peek_event() directly with 100ms, humans are not that sensitive.

The problem with this is it would continuiously update the UI even when not needed, which degrades performance. There's no perfect solution except, I believe, running the code in a separate thread, but I haven't tried that yet.

llc0930 commented 1 month ago

I believe I saw min_refresh_delta = 5 in the config file. Of course, polling consumes performance, but probably few people will stay on the display manager for more than a day.

AnErrupTion commented 1 month ago

I believe I saw min_refresh_delta = 5 in the config file. Of course, polling consumes performance, but probably few people will stay on the display manager for more than a day.

Personally, I don't think that alone is a valid justification for decreasing performance, even if the display manager isn't the most important piece to optimize.