Fix and improvements of battery stats plugin by robert00s · Pull Request #5626 · koreader/koreader

robert00s · 2019-11-22T08:38:53Z

Close: #3031, #2937

What was done:

fix incorrectly show awake and sleeping,
fix incorrectly show charging and discharging,
remove unneeded debug mode,
remove logging to external file,
another method to show indentation text for better translate,
prevent showing values like inf, -inf, nan in estimate time (we show now n/a),
prevent showing values below zero,
show extra confirm box when we want to reset data,
show time in format xxhxxm (hours, minutes) instead only pure minutes,
check at initialization that the device was charging when it was turned off (the battery is larger than at the time of the KO exit),

ToDo:

more tests
show time in format xxdxxhxxm (days, hours, minutes)

Before:

After:

This change is

plugins/batterystat.koplugin/main.lua

Frenzie · 2019-11-22T11:47:37Z

plugins/batterystat.koplugin/main.lua


 function Usage:dumpCharging(kv_pairs)
-    table.insert(kv_pairs, {_("    Estimated hours for charging"), shorten(self:chargingHours())})
+    table.insert(kv_pairs, {string.format("%s%s",INDENTATION, _("Estimated hours for charging")), shorten(self:chargingHours())})


@poire-z Does this work for RTL?

Possibly not - but go ahead with that as it is, it is done the same way in the System Statistics plugin:

There are much more stuff than I expected that will need to be adapted for proper and code-clean RTL mirroring, and I have yet no idea how we'll go with these indented keys in Keyvaluepage...
We'll fix all these, either manually in each of them, or globally if we find a trick to have that done in KeyValuePage or even upper.

As an example, our lazy aligment tricks to use a different padding_left vs padding_right in FrameContainer - would need to be replaced by a HorizontalSpan and the FrameContainer in a HorizontalGroup, so HorizontalGroup is the only one that'd need to deal with mirroring.
Edit: or may be not :) we can just invert padding_left and padding_right in FrameContainer... Still experimenting to see what can/needs to be mirrored for less change outside low level widgets.

My wish is that we should continue to just think LTR when developping, with some simple rules to follow for special cases (like chevron, or string bits to be BD.wrap() before concatenating them - and all should be done automagically by low level widgets when switching to some RTL language.

What I mean is that lazy

some spaces for alignment

can be translated however it needs to be in RTL if they're part of the string, as they are now. If this code would require extra code for inverting then this is a bad change.

Note that a string like الذاكرة الإجمالية (MB) overlooked and left out the spaces, unless it was a conscious decision. This is a Transifex flaw.

left out the spaces.

Oh, right. If I add the leading spaces back in ar_AA/koreader.po, then it looks ok! So, no issue with RTL at having spaces at start for indentation (I thought the bidi algo might push all spaces at end, but it doesn't). So good.

Anyway, the way this PR does it (indentation outside of translated string) is fine. (The way System statistics plugin does it is not.)

Right, but does

" ".."RTL string"

actually put them at the start or at the end?

At the start of the logical order of bytes. So it will be on the right (so at the RTL start) when text is rendered RTL (the bidi algo consider the spaces neutral, so all RTL when next real char is RTL).

There are many things the bidi algo (and/or xtexp.cpp :) do just right - we can still keep thinking LTR, and assume it will work ok with RTL.

Alright, in that case this bit of programmatic cleverness should okay. But in matters of internationalization anything clever is suspect by default. :-)

Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>

NiLuJe

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Frenzie and @robert00s)

robert00s · 2019-11-22T18:36:56Z

plugins/batterystat.koplugin/main.lua, line 30 at r1 (raw file):

Previously, Frenzie (Frans de Jonge) wrote…

Been trying to unify comment style. :-)
local INDENTATION = "   " -- Three spaces.

Done.

Frenzie

Reviewable status: complete! all files reviewed, all discussions resolved

plugins/batterystat.koplugin/main.lua, line 83 at r2 (raw file):

function Usage:dump(kv_pairs)
    table.insert(kv_pairs, {string.format("%s%s",INDENTATION, _("Consumed %")), shorten(self.percentage)})

Btw, we used to have to use string.format because .. wasn't JIT compiled, but that's changed in LuaJIT 2.1 so we can safely use the imo more legible INDENTATION .. _("Consumed").

poire-z

Don't want to dig into the logic and maths :) so, trusting you on these changes :)

poire-z · 2019-11-26T12:11:56Z

plugins/batterystat.koplugin/main.lua

 end

 local BatteryStat = {
    settings = LuaSettings:open(DataStorage:getSettingsDir() .. "/batterstat.lua"),


Can we fix that filename to "battery_stats.lua" ?
(I guess we don't need to care about migration for such transient stuff.)

Of course :)

poire-z · 2019-11-26T12:44:45Z

Merge'able? Was about to, but just saw your:

ToDo:
more tests
show time in format xxdxxhxxm (days, hours, minutes)

robert00s · 2019-11-26T13:00:20Z

Yes, it's ready to merge.
Todo things I'll be do in near future in another PR :)

poire-z · 2019-11-26T14:00:59Z

(A little request @robert00s : can you try deleting your branchs soon after they are merged? Instead of deleting a whole batch of branches a few times a year :) which makes the "sort PR/issues by recently updated" push all the alive issues below all your PRs of the trimester :) - unless you have a reason to do it that way)

Frenzie · 2019-11-26T14:03:25Z

Deleting the branch after merge "updates" the PR? Sounds more like a GH bug/misfeature tbh.

Btw, since we never (?) have more than about a dozen PRs what's the use case precisely? They all fit on one page. :-)

robert00s · 2019-11-26T14:16:55Z

Ok, I'll try to.

poire-z · 2019-11-26T14:32:22Z

Thanks! :)

what's the use case precisely

My landing page is https://github.com/koreader/koreader/issues?q=sort:updated-desc - where I see all the active PRs and issues - and I just have to look for "updated X minutes/hours ago" to see what has happened :) or quickly find out older but recent stuff I planned on commenting on.
(A few days ago, when @robert00s did some clean up, they all went to page 2, and it was hard to find the boundary - but ok, it all settles back after a few days)

Sounds more like a GH bug/misfeature tbh

Surely is - but it has been like that for years :)

- fix incorrectly shown awake, sleeping, charging and discharging, - remove unneeded debug mode and logging to external file, - prevent showing values like inf, -inf, nan in estimated times (we now show "n/a"), and values below zero, - show extra confirm box when we want to reset data, - show time in format xxhxxm instead of only pure minutes, - check at initialization that the device was charging when it was turned off (battery level larger than at the time of exit).

robert00s added 4 commits November 21, 2019 12:39

Fix battery stats plugin

d963128

v1

9d26c8c

fix batterystat_spec

c2867bc

restore stat()

594c425

Frenzie added this to the 2019.12 milestone Nov 22, 2019

Frenzie added bug Plugin labels Nov 22, 2019

Frenzie reviewed Nov 22, 2019

View reviewed changes

plugins/batterystat.koplugin/main.lua Outdated Show resolved Hide resolved

Frenzie reviewed Nov 22, 2019

View reviewed changes

Update plugins/batterystat.koplugin/main.lua

735cd36

Co-Authored-By: Frans de Jonge <fransdejonge@gmail.com>

NiLuJe reviewed Nov 22, 2019

View reviewed changes

Frenzie reviewed Nov 23, 2019

View reviewed changes

string.format to ..

0acff61

poire-z approved these changes Nov 26, 2019

View reviewed changes

rename battery_stats.lua

dc17494

poire-z merged commit 830fc79 into koreader:master Nov 26, 2019

robert00s mentioned this pull request Nov 26, 2019

Battery statistics: "sleeping" time is not identified correctly #2937

Closed

robert00s deleted the battery branch November 26, 2019 14:17

Uh oh!

Conversation

robert00s commented Nov 22, 2019 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Frenzie Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

poire-z Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frenzie Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frenzie Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

poire-z Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

Frenzie Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

Frenzie Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

robert00s commented Nov 22, 2019

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

poire-z Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

robert00s Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

poire-z commented Nov 26, 2019

Uh oh!

robert00s commented Nov 26, 2019

Uh oh!

poire-z commented Nov 26, 2019

Uh oh!

Frenzie commented Nov 26, 2019

Uh oh!

robert00s commented Nov 26, 2019

Uh oh!

poire-z commented Nov 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robert00s commented Nov 22, 2019 •

edited by Frenzie

Loading

poire-z Nov 22, 2019 •

edited

Loading

Frenzie Nov 23, 2019 •

edited

Loading

Frenzie Nov 23, 2019 •

edited

Loading