pop-os / gnome-control-center

Pop!_OS fork of https://git.launchpad.net/~ubuntu-desktop/ubuntu/+source/gnome-control-center
GNU General Public License v2.0
29 stars 10 forks source link

Keyboard backlight control in Gnome Settings #102

Open WatchMkr opened 4 years ago

WatchMkr commented 4 years ago

For laptop models with System76 open firmware (and perhaps other brands if possible) we'd like to expose keyboard backlight controls in GNOME Settings Keyboard section.

We have two KB backlight classes. Those that support on/off and brightness. And those that support on/off, color change, and colors in three regions

ids1024 commented 4 years ago

(and perhaps other brands if possible)

It seems keyboard brightness is already controllable over dbus with the org.gnome.SettingsDaemon.Power.Keyboard schema defined by gnome-settings-daemon, which seems to be just a wrapper arround org.freedesktop.UPower.KbdBacklight provided by upower. Those seem to work on my gaze15.

So it seems it should be fairly easy to support brightness control on a large range of devices. I'm not sure we can provide support for setting color on other models, unless we work to develop a standard API for it...

Edit: The underlying issue here is that setting brightness is a standard thing supported in Linux kernel drivers for keyboards, displays, etc. I don't think there's any standard way to set color, unless there's a user-space library to abstract different models.

ids1024 commented 4 years ago

Actually (and I guess unsurprisingly) it looks like Gnome Control Center can already control keyboard brightness under Power→Power Saving. However we design this, we probably want to avoid duplication, while not breaking support for setting brightness on any devices where it currently works.

RGB is more of a mess. It seems setting RGB (keyboards, etc.) is mostly a mess of random Github projects specific to a device/manufacturer. But there's a fairly recent project called OpenRGB that attempts to unify these. I'm not sure that's useful to us currently, but it's an interesting project to keep track of.

WatchMkr commented 4 years ago

Power Settings is the one place where duplication is good. The intent is to inform customers by showing devices and options that affect power usage. That's why Wifi and Bluetooth are duplicated there as well.

maria-komarova commented 4 years ago

Here are some designs for the Backlight Settings and where they would go inside the Keyboard panel.

I was thinking it might be nice to mention the keys on the keyboard with which a user can quickly cycle through the colors in this Settings but I am not sure if those keys would differ depending on the machine.

Backlight-settings

ahoneybun commented 4 years ago

Most likely we'll want to use the Gtk.Color.Chooser for this like in the screenshot below.

2020-07-14_10-14

maria-komarova commented 4 years ago

Do you have a screenshot for choosing the Custom color?

ahoneybun commented 4 years ago

Here you go!

2020-07-14_10-22

maria-komarova commented 4 years ago

My understanding is that it doesn't quite make sense to enable people to change color based on the region now. We only have a few older products that have that function and the newer ones are going away from having colors by regions.

In terms of the color chooser we can use that if that would make implementation easier. One consideration I had is that there is no black backlight for the keyboard really and the behavior feels weird when one tries to set it to black. I'd rather not have black as an option. But that depends on the implementation considerations.

ids1024 commented 4 years ago

In terms of the color chooser we can use that if that would make implementation easier

GtkColorChooserDialog is built into Gtk, so it's certainly simpler to use that, rather than having to implement something new.

I'm not sure exactly how difficult it would be to implement the custom color wheel. But it generally can be somewhat awkward to create custom widgets that aren't a straightforward combination of standard ones, and I don't think there's a standard gtk widget that would help with selecting a particular point on the color wheel.

But I'm not sure exactly how that would be done. I suppose I'd look at how GtkColorChooser is implemented.

One consideration I had is that there is no black backlight for the keyboard really and the behavior feels weird when one tries to set it to black.

Logically, the left to right axis on that screenshot would map to brightness (all the way at the black end would turn it off). But since brightness is probably handled completely separately from color, I guess the result is that we'd essentially ignore that axis? I can see how that might not provide an ideal experience.

ids1024 commented 4 years ago

Logically, the left to right axis on that screenshot would map to brightness[...]

Or rather, to use the proper color theory terminology:

GtkColorChooser allows the user to select any color in the HSV (hue, saturation, value) color space. The bar on the left selects the hue. Then in the box on the right, saturation runs from bottom to top, and value from left to right.

But "value" in the HSV color space is just another word for brightness. So if we are not using this color chooser for controlling brightness, but only selecting the color, we ideally want an interface, like the color wheel mockup, that selects just the hue and saturation, without a value axis.

maria-komarova commented 4 years ago

Yes, those are my thoughts on brightness and what we would ideally want to have in the interface.

ids1024 commented 4 years ago

It seems for GtkColorChooser, this is implemented with rendering calls to Cairo. It doesn't look too bad, but certainly less trivial than using an existing widget. Not sure how easy it would be to render a color wheel in Cairo.

The implementation in Gtk 4 is completely different, using features from that version of Gtk, so this is something that would probably warrant a re-write whenever that is released.

Edit: Or maybe those aren't the right links... there are several private widgets involving color, and I'm not entirely sure how they fit together. But the basic point is the same.

ids1024 commented 4 years ago

It looks like with keyboard-color-chooser (and thus presumably the underlying kernel API), setting the color to black does have the effect of turning off the backlight, and setting to between red and black (for instance) results in half brightness even if the brightness control is at 100%.

So the value component wouldn't be ignored, unless we do so explicitly. But I guess what I'm describing may not be an ideal interface/behavior...

maria-komarova commented 4 years ago

Updated mockups with the somewhat nicer UI. We can use gtk-color-chooser for adding or editing the color dialog since that seems to be an existing pattern. There might be that awkward behavior if someone chooses black but it might still be better than trying to implement a custom selector. Backlight-settings (1)

ids1024 commented 4 years ago

Currently, the UI mockup shows setting only one color. So this raises a few questions, after trying to implement the first part of this (https://github.com/pop-os/system76-power/pull/177):

maria-komarova commented 4 years ago

Thanks for the update.

As mentioned by @WatchMkr in the description for this issue, some System76 models support separate colors for three regions: left, center, and right. Do we intend to be able to control them separately in this interface?

After investigating the options more closely we came to the understanding that it wouldn't quite make sense to enable people to change color based on the region now. We only have a few older products that have that function and the newer ones are moving away from having colors by regions.

I assume per-key RGB is out of scope for this interface.

That would probably be going too far in terms of customization

What if there are multiple keyboards connected? A user is likely to have the Launch keyboard as well as their laptop's builtin keyboard. Should there just be one control to set both? Does plugging in the external keyboard disable the builtin keyboard's backlight?

Good questions. I believe that it should work similar to how display settings work in terms of UI but I will need to incorporate that into mockups. I will try to update the mockups for that today, tomorrow at the latest.

If our interface exposes only a limited subset of what certain hardware is capable of, a user who really cares about it should be able to disable any automatic behavior to control the backlight manually through other software (terminal commands, etc.).

I am not sure if they should be able to disable the color settings in the UI. Will that be a necessity for them to have their own customization through the terminal or other software? Would we then disable the special keys on the keyboard that allow one to cycle through the colors if someone manually set up the regions on those older models?

ids1024 commented 4 years ago

For an external keyboard, it seems to me it might be nice if plugging it in automatically disables the builtin keyboard's backlight and sets the color and brightness of the external keyboard to what the builtin keyboard had set, and unplugging does the reverse.

Not sure if that's what most users would want/expect.

I am not sure if they should be able to disable the color settings in the UI. Will that be a necessity for them to have their own customization through the terminal or other software?

This probably shouldn't be a huge issue. We just would need to make sure that just opening the keyboard panel (for instance) doesn't mess with a non-standard configuration of the backlight. And if we have a process running in the background dealing with the backlight, it won't do anything that would override custom colors. And if it does, it should be possible to disable though (though maybe only through a terminal command.)

Probably not something that affects the broader design at the moment. Depending on what we implement, we'll see if anything needs to be done to not cause issues here.

maria-komarova commented 4 years ago

I like the idea of automatically setting the backlight color of the external keyboard to the same setting as the built-in one. But I am not convinced we should disable the built-in keyboard. I think I'd like to understand a bit better what happens in terms of backlight when people use external keyboard. I was never bothered myself by the backlight on the built-in keyboard myself while using the external one. But I guess it might get annoying depending on the color. So maybe that is the case when we would want to give people an option to disable the backlight. If we disable the backlight on the built-in keyboard by default that might cause some confusion and people might even think that something broke. I need to think a bit more about it.

maria-komarova commented 4 years ago

@ids1024 do you know what happens now with the keyboard brightness Setting when someone connects an external keyboard? It will probably be something to consider as well.

ids1024 commented 4 years ago

It doesn't seem to do anything when a keyboard is plugged in; the brightness stays the same. But that's given the external keyboard doesn't have a backlight that would be controllable by Gnome Control Center.

It seems UPower only allows getting/setting a single brightness; it doesn't provide per-device brightness. I'm not sure exactly what the behavior is. I assume it works with some USB keyboards, so it would be possible to have multiple keyboards with controllable backlights.

ids1024 commented 4 years ago

It looks like the up_kbd_backlight_find() function in UPower just selects the first device it finds with a keyboard backlight.

So with two keyboards, I believe Gnome Control Center would have only one keyboard backlight slider, which would control only one of the keyboards, whichever UPower happened to find first.

...which isn't great, so I guess we'll need to change this behavior.

maria-komarova commented 4 years ago

Yes, that doesn't sound like a great behavior. In terms of the UI, I was thinking if we can just have tabs that would provide a way to change settings for each Keyboard separately. I guess if we decide to disable the built-in keyboard when someone plugs in the external one, we might just put the brightness slider in the off position. We would also need some message telling the user that the built-in keyboard backlight was turned off to save power. Not yet sure where would that message go, probably above the brightness control on the Built-in Keyboard tab. But I am for trying that default behavior.

Keyboard

ids1024 commented 4 years ago

It may be difficult to get a user-friendly name for each device (though we can do something for our own hardware, at least). Which is awkward...

I'm not really sure what external keyboards have brightness control that works through UPower, though the last commit to the keyboard backlight code in UPower suggests that some external keyboard works.

maria-komarova commented 4 years ago

If they don't have brightness control through UPower, would they still have color control?

ids1024 commented 4 years ago

UPower doesn't have an interface for controlling the color of the backlight. There doesn't really seem to be a standard way to do that. Manufacturers of RGB peripherals and components just each provide their own proprietary Windows software. Then there are some small open source projects for controlling the RGB lighting on devices from a particular manufacturer. It seems a project called OpenRGB is becoming popular, for supporting a variety of devices.

But it's a GUI written in C++ and Qt, without a library, and isn't packaged by Ubuntu. Perhaps we could adapt it to work for our use, but likely we'll want to stick to supporting color control only on System76 hardware.

maria-komarova commented 4 years ago

Thanks for clarifying. Yes, it seems reasonable that we will want to stick with supporting color control only on our hardware in that case. Coming back to the external keyboard question, I think that would mean that the UI with tabs would be only for the cases when there is something to control which will probably limited to our hardware. I am not sure we would be able to expose external keyboards with any other hardware in that case. Which means we probably shouldn't try to disable the backlight in those scenarios.

ids1024 commented 4 years ago

And then there's the question of what we want to persists across reboots. I assume the list of colors is persisted.

Should the color and brightness be persisted across reboots? Per device? One thing to note is that if the color and brightness are set when Pop!_OS starts, that won't affect early in the boot process.

(On open firmware systems, at least, we should be able to control behavior of the built-in keyboard backlight from the very start of the boot process. But I don't know if we necessarily want the control panel to be changing settings in the firmware here.)

ids1024 commented 4 years ago

Current implementation:

Screenshot from 2020-08-11 15-38-25

It works, for changing the backlight color and for adding and removing colors. Some of the styling could use a bit of work, and it should look less weird actually integrated into Gnome Control Center. And anything involving persisted settings and multiple devices still needs to be done.

ids1024 commented 4 years ago

I have it running in Gnome Control Center. It looks a bit better there, and I've tweaked a couple things about the style, though it could still use some work. And the doesn't yet include a brightness slider or support for multiple devices.

I've written some code for displaying a hue/saturation color wheel and selecting a point on the wheel. Rendering it and handling mouse events properly was easier than I expected it might be, so we should be able to use that rather than the standard GtkColorChooserDialog.

Screenshot from 2020-08-13 09-06-59

Screenshot from 2020-08-13 08-53-26

ids1024 commented 4 years ago

And that, embedded in a simple dialog, which works:

Screenshot from 2020-08-13 14-39-55

maria-komarova commented 4 years ago

The color wheel looks amazing!! That's great that it turned out to be easier than expected!

We can work on the checkmark. The colored border would probably work fine here. We can also add the rectangle field that shows the chosen color more fully (on the very first mockup in the issue below the wheel). I wonder if we should include the ability for people to use a hex code or RGB. Not sure if that might be overcomplicating the design.

As implemented, it looks pretty funny when all but the last color is removed. I suppose I'll see if there's an easy way to force the popover to have the same width as when there are more colors. That might look better. Absolutely. If we can have the fixed width, that would be best. That was the original idea.

I was considering having the reset button. It complicates the design though and I am not sure how many people would use it. We can think more about it once we get a bit closer and have a chance to try it ourselves.

ids1024 commented 4 years ago

We can also add the rectangle field that shows the chosen color more fully (on the very first mockup in the issue below the wheel).

Right. It should definitely have something like that. That design should probably work.

I wonder if we should include the ability for people to use a hex code or RGB. Not sure if that might be overcomplicating the design.

I'd like to support that. But it could get a bit weird given the UI we're exposing doesn't actually allow arbitrary RGB values, only hue and saturation. So if someone enters the RGB code 7f0000, which is halfway between red and black, do we just silently ignore the darkness, and treat it as equivalent to ff0000? What about 000000 (black)? Do we just interpret that as white?

maria-komarova commented 4 years ago

Yes, you are right, it would get weird. I think it is fine to keep it simple with the wheel as the selector. It would give a more accurate color setting that way probably anyway.

ids1024 commented 4 years ago

Still some style details that could be improved, but it now has an interface for switching devices:

(The names of the two "keyboards" of course are placeholders).

Screenshot from 2020-08-27 14-43-54

cleveHEX commented 3 years ago

It looks good, any chance to see this in production?

crawfxrd commented 1 year ago

Do we still need/want this?