Page MenuHomePhabricator

Backlight Gadget slider fixes
ClosedPublic

Authored by jf_simon on Jan 11 2020, 11:38 AM.

Details

Summary

What i have done:

  • Open slider will update using mouse wheel over light bulb
  • Open slider will update using keybindings
  • Slider shows now a value between 1 and 100 (no double value is displayed anymore)
  • Removed unnecessary code for shelf module
  • removed code for buggy popup using keybindings (may add later an option for showing popup on backlight change)

I am still learning. please consider :)

Greetings Simon

Diff Detail

Repository
rE core/enlightenment
Lint
Lint Skipped
Unit
Unit Tests Skipped
jf_simon created this revision.Jan 11 2020, 11:38 AM
jf_simon requested review of this revision.Jan 11 2020, 11:38 AM
bu5hm4n added inline comments.Jan 13 2020, 2:16 AM
src/modules/backlight/gadget/backlight.c
38

Can you elaborate why this is changed ?

jf_simon added inline comments.Jan 13 2020, 3:01 AM
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.

jf_simon updated this revision to Diff 28810.Feb 5 2020, 12:29 AM

rebase to latest master backlight changes

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?

raster added a comment.Feb 5 2020, 3:01 AM

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?

jf_simon marked 3 inline comments as done.Feb 5 2020, 5:11 AM
jf_simon added inline comments.
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.

jf_simon updated this revision to Diff 28859.Feb 5 2020, 5:13 AM

update diff according to comments

jf_simon marked 3 inline comments as done.Feb 5 2020, 5:13 AM
raster accepted this revision.Feb 5 2020, 6:15 AM

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.

This revision is now accepted and ready to land.Feb 5 2020, 6:15 AM

Simon Tischer <simon@t-tischer.de>

thanks raster