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

Device:Android: call "_decideFrontlightState" to keep "is_fl_on" in sync #10731

Merged
merged 1 commit into from Jul 23, 2023

Conversation

hasezoey
Copy link
Contributor

@hasezoey hasezoey commented Jul 23, 2023

re #10725

This PR is so that variable is_fl_on is also kept in-sync when fl_intensity is being set, this could maybe be de-duplicated by always calling it after the both if's.

Why?

because thing that depend on is_fl_on do not work correctly after just fl_intensity being set, like BasePowerD:isFrontlightOn (by extension also BasePowerD:isFrontlightOff) and so in turn things like BasePowerD:toggleFrontlight call the wrong function.

Example:
(assuming #10726 is merged, makes it easier to explain)

initial state:
fl_intensity: 50
is_fl_on: true

then you go to the lights dialog, change brightness to 0 (fronlight being off) and then just fl_intensity is set to 0, but is_fl_on stays true - which is wrong, and so if you then do something like event ToggleFrontlight it will go the path of Frontlight Disabled, even though the frontlight was already disabled

@pazos i hope this examples makes it more clear what i meant with #10725 (comment), i have go the path of or to use powerd:_decideFrontlightState()
to make it less forgettable in the future maybe refactor this with one of the other options listed in the mentioned comment (like adding a helper function for only internal state)


This change is Reviewable

@hasezoey
Copy link
Contributor Author

hasezoey commented Jul 23, 2023

just to be on the same page, because of (#10725 (comment))

I'm not against fixing issues, but I would like to keep workarounds buried in android-luajit-launcher. If the entire problem here is is_fl_on just make a method as part on the LightsController class and let drivers deal with it.

is_fl_on = false, -- whether the frontlight is on

function BasePowerD:isFrontlightOn()
return self.is_fl_on
end

function BasePowerD:toggleFrontlight(done_callback)
if not self.device:hasFrontlight() then return false end
if self:isFrontlightOn() then
return self:turnOffFrontlight(done_callback)
else
return self:turnOnFrontlight(done_callback)
end
end

@pazos
Copy link
Member

pazos commented Jul 23, 2023

Ok, thanks for the lengthy explanation.

Now makes sense to me.

@pazos pazos merged commit b1109a7 into koreader:master Jul 23, 2023
3 checks passed
@hasezoey hasezoey deleted the setFLOn branch July 23, 2023 11:35
@Frenzie Frenzie added this to the 2023.07 milestone Jul 23, 2023
Copy link
Member

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first hunk makes sense (c.f., e.g., BasePowerD's init, or even simply setIntensity), but I'd stash the second one away in the imp.

@@ -510,6 +511,7 @@ function Device:_showLightDialog()
elseif action == C.ALIGHTS_DIALOG_CANCEL then
logger.dbg("Dialog Cancel, brightness: " .. self.powerd.fl_intensity)
self.powerd:setIntensityHW(self.powerd.fl_intensity)
self.powerd:_decideFrontlightState()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would possibly move that one inside Android's setIntensityHW implementation. (We do the same on a few other platforms, e.g., Kobo).

Frenzie pushed a commit that referenced this pull request Oct 12, 2023
…HW" (#10737)

re #10731 (comment)

This PR changes so that androids implementation of `setIntensityHW` always calls `_decideFrontlightState`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants