What i have done:
I am still learning. please consider :)
Greetings Simon
| Lint Skipped |
| Unit Tests Skipped |
| src/modules/backlight/gadget/backlight.c | ||
|---|---|---|
| 38 | Can you elaborate why this is changed ? | |
| src/modules/backlight/gadget/backlight.c | ||
|---|---|---|
| 38 | The minimum value from the slider was 0.05. I changed this also to 1. So we have a range from 1 to 100. 0 would be a absolute black display. So set it here also to min 0.01, then min for slider is 1. | |
why not a range of 0 to 100? you know that 0 could be black or could just be very dim. it varies from hardware to hardware. also lowest level backlight could be hit at 10/100 if the backlight device only has 9 steps... i've seen backlights with 8 and 5 levels before... others i've seen with 8000 levels or so... others with 255, some with 100 (at the kernel interface). the whole backlight system just simplifies it to 0.0 to 1.0 at the api level in e as there is no way to know when/if the screen actually goes totally off or if 0 is just a very dim value without experimenting on each and every device. if we're suing xrandr as the controller (which we do if it offers it in x) then nothing in e sees even the number of available levels - it's also abstracted out like inside of e by the xserver/xrandr.
the only solution that would work in a general sense is to be able to configure a minimum level for the backlight gadget. you don't want a min level in the core as backlight is used for fading to/from darkest possible hardware level for various reasons. so why not just make it 0 to 100 and deal with the "my screen goes black and i can;'t see it" another way... :) not necessarily in this patch.
ok, with this background information i agree with you to set the range to 0 - 100. :)
i will update the diff according
can you agree with the other changes i made?
ok. sounds good. let me actually just make a few comments in the code :)
| src/modules/backlight/gadget/backlight.c | ||
|---|---|---|
| 41 | Can you maybe the above elm_slider_value_set(inst->o_slider, inst->val * 100); Being a bit nit-picky as an "education exercise". making code flow/format the same makes it easier to read and maintain as it's consistent. :) | |
| 53 | Also above - spaces inst->val = elm_slider_value_get(inst->o_slider) / 100; | |
| 100 | 0 to 100. :) perhaps be consistent and make this 100 not 100.0 or make the other 100's floats too and make them 100.0 | |
| 103 | also spaces and consider 100 vs 100.0: elm_slider_value_set(o, inst->val * 100); | |
| 150 | this change in this foreach... how was it buggy? | |
| src/modules/backlight/gadget/backlight.c | ||
|---|---|---|
| 150 | if you set the keybinding for the gadget under "screens->Backlight Controls" and press the keybindings. the popup will appears and disappears with ever press. | |
that's better :) now what do you want me to do with this. if i snarf this from phab using arcanist the commit ends up being from me n the log (not you). Can you give me an email line to use as author like:
Name Laat <email@domain.com>
so i get the right one for you? :)
| src/modules/backlight/gadget/backlight.c | ||
|---|---|---|
| 150 | Ah. yes it would. it probably should have a timer to hide the popup after time X and otherwise keep it up etc. removing the popup from the action for now removes that issue indeed. | |