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

A small round of fixes #9676

Merged
merged 7 commits into from
Oct 24, 2022
Merged

A small round of fixes #9676

merged 7 commits into from
Oct 24, 2022

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Oct 23, 2022


This change is Reviewable

Because of floating point computery math stuff.

Regression since koreader#9609
c.f., koreader#9609 (comment)
Which allows us to exit the screensaver on, well, any key press ;o).
And simplify the few we do catch by using aliases instead of duplicated
functions;).
@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 23, 2022

(Rebase me)

for i = 1,5 do
self:setIntensityHW(math.ceil(self.fl_min + ((self.fl_intensity * (1/5)) * i)))
for i = 1, 5 do
self:setIntensityHW(math.ceil(self.fl_min + (self.fl_intensity / 5 * i)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self:setIntensityHW(math.ceil(self.fl_min + (self.fl_intensity / 5 * i)))
self:setIntensityHW(math.ceil(self.fl_min + (self.fl_intensity / 5 * i))) -- @zwim don't change me :-P

Copy link

Choose a reason for hiding this comment

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

I dont know if it is important, but after applying these changes, Toggling frontlight on Kobo Libra 2 works as it should!
Thank You very much! :)

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 23, 2022

  • [This is properly disabled when using the child lock pattern mode, because @poire-z rules ;)]

Actually... almost ;o). key_events & ges_events cannot be nil, we run an unchecked pairs on 'em. I'll fix that tomorrow ;).

If you've ever enabled the main loop debugging, you'll know that
actually dumping the full window stack was *hilarious*.
Just print table counts, it's often good enough to debug what's
happening in the exceedingly rare cases you need this ;).
Also, it'll actually be readable, unlike the previous insanity ^^.
@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 24, 2022

Ping @zwim for d034e1a to confirm I didn't fuck it up ;).

@poire-z
Copy link
Contributor

poire-z commented Oct 24, 2022

Does the frontlight issue fix deserves a v2022.10.1 (not affected, can't judge criticity) ?
Or can we just go at merging waiting PRs ?

@@ -171,7 +171,7 @@ end
function time.split_s_us(time_fts)
if not time_fts then return nil, nil end
local sec = math.floor(time_fts * FTS2S)
local usec = math.floor(time_fts - sec * S2FTS) * FTS2US
local usec = math.floor((time_fts - (sec * S2FTS)) * FTS2US)
Copy link
Contributor

@zwim zwim Oct 24, 2022

Choose a reason for hiding this comment

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

Good catch. The original line is buggy. Your version works. The innermost parentheses are superfluous. So maybe write

local usec = math.floor((time_fts - sec * S2FTS) * FTS2US)

The good news is, that this bug is not noticeable by now, as PRECISION can be stored in a double without precision loss; US2FTS is exactly 1 and FTS2US is exactly 1, too. But for future (if someone wants to tune the thing by using a different PRECISION value (e.g. a power of 2), this buggy line would cause an issue).

@NiLuJe
Copy link
Member Author

NiLuJe commented Oct 24, 2022

Does the frontlight issue fix deserves a v2022.10.1 (not affected, can't judge criticity) ?

That's a good question. It breaks toggle off on a not insignificant number of frontlight values, which is mildly annoying, but can be worked-around by just changing the fl value by +/- 1, so, eeeeh.

Might be annoying to field those issues for a month, though ;).

The us part wasn't actually truncated properly.
@poire-z
Copy link
Contributor

poire-z commented Oct 24, 2022

OK, so it does not prevent anybody from using the software. We mostly did some rerelease when it crashed at start I guess. With v2022.10, they can upgrade to dev channel and get it fixed. Or live with it, they deserve it :) for not helping us fixing stuff by being on the dev channel :)

We can remove "Chinese, Japanese and Korean typography: some questions" from the pinned issue if we want a slot to pin the frontlight issue until v2022.11, and may be avoid some duplicates opening.

Copy link
Member Author

@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.

Reviewed 14 of 15 files at r1, 3 of 4 files at r2, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie, @pazos, @poire-z, and @zwim)

@NiLuJe NiLuJe merged commit f04beb1 into koreader:master Oct 24, 2022
@Frenzie Frenzie added this to the 2022.11 milestone Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Libra2 Frontlight toggle doesn't turn off light with some values.
5 participants