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

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

Open
1 task done
Blayung opened this issue Aug 1, 2024 · 30 comments
Open
1 task done

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

Blayung opened this issue Aug 1, 2024 · 30 comments
Labels
bug This issue or pull request discusses a bug

Comments

@Blayung
Copy link

Blayung commented Aug 1, 2024

Pre-requisites

  • I have looked for any other duplicate issues

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

@Blayung Blayung added the bug This issue or pull request discusses a bug label Aug 1, 2024
@AnErrupTion
Copy link
Collaborator

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

@Blayung
Copy link
Author

Blayung commented Aug 1, 2024

So should this issue stay?

@AnErrupTion
Copy link
Collaborator

So should this issue stay?

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

@Blayung
Copy link
Author

Blayung commented Aug 1, 2024

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
Copy link
Collaborator

They mention it right there: #607 (comment)

@llc0930
Copy link
Contributor

llc0930 commented Aug 1, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

llc0930 commented Aug 1, 2024

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
Copy link
Collaborator

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
Copy link
Author

Blayung commented Aug 1, 2024

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

@llc0930
Copy link
Contributor

llc0930 commented Aug 1, 2024

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
Copy link
Author

Blayung commented Aug 1, 2024

But I didn't update.

@llc0930
Copy link
Contributor

llc0930 commented Aug 1, 2024

But I didn't update.

So mysterious.

@AnErrupTion
Copy link
Collaborator

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
Copy link
Contributor

llc0930 commented Aug 1, 2024

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
Copy link
Collaborator

By the way, I just saw #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
Copy link
Contributor

llc0930 commented Aug 1, 2024

It should be before #78.

@llc0930
Copy link
Contributor

llc0930 commented Aug 1, 2024

This feature has existed at least since ce745bf.

@llc0930
Copy link
Contributor

llc0930 commented Aug 1, 2024

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
Copy link
Contributor

llc0930 commented Aug 1, 2024

But I didn't update.

So maybe you have animations or a clock turned on?

@AnErrupTion
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

Blayung commented Aug 2, 2024

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
Copy link
Contributor

llc0930 commented Aug 2, 2024

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
Copy link
Contributor

llc0930 commented Aug 3, 2024

Hope these can help you:

@AnErrupTion
Copy link
Collaborator

AnErrupTion commented Aug 3, 2024

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
Copy link
Contributor

llc0930 commented Aug 3, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

llc0930 commented Aug 3, 2024

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request discusses a bug
Projects
None yet
Development

No branches or pull requests

3 participants