[DoubleSpinWidget, SpinWidget] change values with page-turn buttons by Commodore64user · Pull Request #13208 · koreader/koreader

Commodore64user · 2025-02-04T01:13:02Z

what's new

This pull request includes changes to the DoubleSpinWidget and SpinWidget classes. The modifications enhance the functionality of these widgets by adding new key events to support value changing using the device's page-turn buttons.

enhancements to `DoubleSpinWidget`:

Added key events to support adjusting widget values.
Replaced local variables with instance variables for left_widget and right_widget.
Introduced the onDoubleSpinButtonPressed method to handle page-turn button presses for adjusting widget values.

enhancements to `SpinWidget`:

Added key events to support adjusting widget values.
Replaced local variables with instance variables for value_widget.
Introduced the onSpinButtonPressed method to handle page-turn button presses for adjusting widget values.

others

Removed number of arguments needed to pass to changeValue method of NumberPickerWidget.
closes FR: Use Page-turn buttons to increase/decrease widget settings #11821

This change is

Commodore64user · 2025-02-04T23:01:37Z

Surprised to see @poire-z hasn't shown his visage around here as pear usual, pearhaps he's been busy. door closes.

poire-z · 2025-02-04T23:40:57Z

You're pearobably right. No pear review.

But no comment after a quick glance, looks ok, but I'm not really familiar with these widgets.

hius07 · 2025-02-05T11:31:36Z

koreader/frontend/ui/widget/numberpickerwidget.lua

Line 81 in a15c294

    
           self.value = self:changeValue(self.value, self.value_step, self.value_max, self.value_min, self.wrap)

Not a perfect old code, passing four redundant args. Step only is enough.

Commodore64user · 2025-02-05T13:07:20Z

want me to do this?

function NumberPickerWidget:changeValue(step)
    local value
    if self.value_index then
        self.value_index = self.value_index + step
        if self.value_index > #self.value_table then
            self.value_index = self.wrap and 1 or #self.value_table
        elseif self.value_index < 1 then
            self.value_index = self.wrap and #self.value_table or 1
        end
        value = self.value_table[self.value_index]
    else
        value = self.value + step
        if value > self.value_max then
            value = self.wrap and self.value_min or self.value_max
        elseif value < self.value_min then
            value = self.wrap and self.value_max or self.value_min
        end
    end
    return value
end

hius07 · 2025-02-05T13:23:30Z

Sure, no sense to pass the module's properties within itself.

hius07 · 2025-02-05T15:12:04Z

                self.value_max = self:getDaysInMonth(self.date_month:getValue(), self.date_year:getValue())
            end
-            self.value = self:changeValue(self.value, self.value_step * -1, self.value_max, self.value_min, self.wrap)
+            self.value = self:changeValue(self.value_step * -1)


-self.value_step

it looks quite rubbish though, I would say it's better this way (step * -1) vs (-step)

Commodore64user · 2025-02-05T17:52:51Z

I added a part 2 to this because that one includes a few other things.

Frenzie · 2025-02-06T23:24:44Z

I didn't follow the discussion about local in detail.

hius07 · 2025-02-07T17:06:25Z

 }

+local left_widget
+local right_widget


I suppose it's better to declare them in the beginning of init() and pass as args instead of the strings.

Having them as module local means they would be shared by all DoubleSpinWidget instances. In the case we have more than ont usable (which may not happen with koreader), they would be replaced and shit would happen.
So, this is conceptually wrong.

Either they should be hold in self. (the instance that has them), or function local and passed as arguments.

Passing as args doesn't require additional strings ("left_widget"...) and redundant check in onDoubleSpinButtonPressed.

Either they should be hold in self. (the instance that has them), or function local and passed as arguments.

just to clarify, they were self before, they were self at the ~~first~~ second commit... I am moving them to init then, but cannot pass them directly to the key_events as arguments because they are nil at that moment.

./luajit: frontend/ui/widget/doublespinwidget.lua:375: attempt to index local 'target_widget' (a nil value)

and of course

Checking frontend/ui/widget/doublespinwidget.lua 24 warnings frontend/ui/widget/doublespinwidget.lua:63:11: unused variable left_widget frontend/ui/widget/doublespinwidget.lua:64:11: unused variable right_widget frontend/ui/widget/doublespinwidget.lua:105:5: setting non-standard global variable left_widget frontend/ui/widget/doublespinwidget.lua:116:34: accessing undefined variable left_widget frontend/ui/widget/doublespinwidget.lua:117:5: setting non-standard global variable right_widget frontend/ui/widget/doublespinwidget.lua:128:34: accessing undefined variable right_widget frontend/ui/widget/doublespinwidget.lua:129:5: mutating non-standard global variable left_widget frontend/ui/widget/doublespinwidget.lua:130:28: accessing undefined variable right_widget frontend/ui/widget/doublespinwidget.lua:132:5: mutating non-standard global variable right_widget frontend/ui/widget/doublespinwidget.lua:133:21: accessing undefined variable left_widget frontend/ui/widget/doublespinwidget.lua:144:9: accessing undefined variable left_widget frontend/ui/widget/doublespinwidget.lua:152:9: accessing undefined variable right_widget frontend/ui/widget/doublespinwidget.lua:222:21: mutating non-standard global variable left_widget frontend/ui/widget/doublespinwidget.lua:223:21: mutating non-standard global variable right_widget frontend/ui/widget/doublespinwidget.lua:224:21: accessing undefined variable left_widget frontend/ui/widget/doublespinwidget.lua:225:21: accessing undefined variable right_widget frontend/ui/widget/doublespinwidget.lua:236:45: accessing undefined variable left_widget frontend/ui/widget/doublespinwidget.lua:236:69: accessing undefined variable right_widget frontend/ui/widget/doublespinwidget.lua:257:68: accessing undefined variable left_widget frontend/ui/widget/doublespinwidget.lua:258:40: accessing undefined variable right_widget frontend/ui/widget/doublespinwidget.lua:260:35: accessing undefined variable left_widget frontend/ui/widget/doublespinwidget.lua:261:36: accessing undefined variable right_widget frontend/ui/widget/doublespinwidget.lua:372:60: accessing undefined variable left_widget frontend/ui/widget/doublespinwidget.lua:372:75: accessing undefined variable right_widget Checking frontend/ui/widget/spinwidget.lua 15 warnings frontend/ui/widget/spinwidget.lua:58:11: unused variable value_widget frontend/ui/widget/spinwidget.lua:96:5: setting non-standard global variable value_widget frontend/ui/widget/spinwidget.lua:112:32: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:115:9: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:155:24: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:156:25: mutating non-standard global variable value_widget frontend/ui/widget/spinwidget.lua:158:25: mutating non-standard global variable value_widget frontend/ui/widget/spinwidget.lua:160:21: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:170:48: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:182:48: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:209:72: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:211:48: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:351:5: mutating non-standard global variable value_widget frontend/ui/widget/spinwidget.lua:351:26: accessing undefined variable value_widget frontend/ui/widget/spinwidget.lua:352:5: accessing undefined variable value_widget

local a = { x = 0 } local b = { args = { a, 1 } } a.x = 1 print(b.args[1].x)

1

I see, later we do not modify self.left_widget, but reassign it, and it breaks the reference.
You are right.

But we're not doing a.x = 1 in this PR.
We're doing a = { y = 2 }.

local a = { x = 0 } local b = { args = { a, 1 } } a = { x = 1 } print(b.args[1].x)

0

thank you @poire-z for saying (loquaciously) the same thing I said here #13208 (comment) ;). I guess I wasn't "wrong" after all.

#13208 (comment) should be corrected like so, ;)

Your point is ~~not~~ correct.
~~But even better: keep it local, and declare in the beginning of init().~~

poire-z

The stuff I joined the discussion about reads ok.
I'll let others approve the feature and tweaks.

hius07 · 2025-02-08T05:01:10Z

Suggesting locals, I forgot about poor architecture of the spin widget.
In update() (ie when changing values) it recreates left and right widgets from scratch.
Usually we build a widget in init(), and then update it and its subwidgets without rebuilding.
For example

koreader/frontend/ui/widget/titlebar.lua

Lines 456 to 461 in 68cfd96

    
           function TitleBar:setLeftIcon(icon) 
        
               if self.has_left_icon then 
        
                   self.left_button:setIcon(icon) 
        
                   UIManager:setDirty(self.show_parent, "ui", self.dimen) 
        
               end 
        
           end

poire-z · 2025-02-08T08:36:31Z

Usually we build a widget in init(), and then update it and its subwidgets without rebuilding.

I'm not certain it's that "usually". There are lots of places where we rebuild in some :update(), without having to add and use :setXyZ() methods to just not have to recreate a widget. I know you (and @NiLuJe for DictQuickLookup) did that kind of stuff, but it doesn't feel "poor" to not do that, moreover for such minor widgets such as these SpinWidget which are not heavily used.

Frenzie · 2025-02-09T17:44:57Z

I'm with poire-z. I don't mind.

NiLuJe · 2025-02-10T14:21:23Z

Ditto. I conceptually prefer it when that's the case, but it's often not trivial ;).

Commodore64user · 2025-02-12T11:20:45Z

Hot off the press—give it a whiff! ;)

@Frenzie and @NiLuJe still waiting for your reviews

Frenzie · 2025-02-14T17:38:26Z

Fine by me, but I'm not sure if the * -1 would potentially set a bad precedent where it actually matters.

Commodore64user · 2025-02-14T19:36:46Z

I'm not sure that is a legitimate concern though, I mean, the "bad precedent" has existed for a while now ;), nothing to do with me.

Frenzie · 2025-02-14T20:37:49Z

One of the biggest lessons I've had to learn here is that "oh well, what does it hurt?" means that people will be copying it all over the place. What you say there is an argument for nipping it in the bud, not in favor.

But what I meant is that I don't know if it actually is or isn't a bad precedent.

In LuaJIT it also doesn't seem to be a bad precedent, unless someone dislikes the style.

$ cat bla.lua
bla = -10
$ cat bla2.lua
bla = 10 * -1
$ luajit -b bla.lua bla.out; luajit -b bla2.lua bla2.out
$ ls -lah
total 40K
drwxr-xr-x  2 frans frans 4.0K Feb 14 21:32 .
drwxrwxrwt 26 root  root   20K Feb 14 21:31 ..
-rw-r--r--  1 frans frans   14 Feb 14 21:32 bla2.lua
-rw-r--r--  1 frans frans   30 Feb 14 21:34 bla2.out
-rw-r--r--  1 frans frans   10 Feb 14 21:31 bla.lua
-rw-r--r--  1 frans frans   30 Feb 14 21:34 bla.out
$ cmp -l bla.out bla2.out

Commodore64user · 2025-02-17T21:22:29Z

Has anyone got any other comments?

Outdated.

…oreader#13208)

Commodore64user added 7 commits February 3, 2025 16:57

use PgTurn buttons to change spinWidget

f6a6f12

store references to numberpicker widgets

c83a72f

create event handler for spinwidget

016426e

don't re-invent the wheel, Buh-dum-tss

c42efa9

forgo not needed local variables

68f718f

add documentation

deba81d

fix comments

20cd9e5

Frenzie reviewed Feb 4, 2025

View reviewed changes

Comment thread frontend/ui/widget/doublespinwidget.lua Outdated

Frenzie added this to the 2025.02 milestone Feb 4, 2025

code review

cf2b625

Frenzie reviewed Feb 4, 2025

View reviewed changes

Comment thread frontend/ui/widget/spinwidget.lua Outdated

use self.value_step directly

636f79a

Commodore64user commented Feb 4, 2025

View reviewed changes

Comment thread frontend/ui/widget/doublespinwidget.lua

and the other one too

564db7a

Frenzie reviewed Feb 4, 2025

View reviewed changes

Comment thread frontend/ui/widget/doublespinwidget.lua Outdated

Commodore64user added 2 commits February 5, 2025 11:02

remove inter local var

1a0a274

don't assume the state of self.wrap

ba6943b

refactor changeValue() method

02f6990

hius07 reviewed Feb 5, 2025

View reviewed changes

Commodore64user mentioned this pull request Feb 5, 2025

[DateTimeWidget] add key_events and fix setting dates #13214

Merged

Commodore64user requested a review from Frenzie February 6, 2025 23:13

keep pickers local

ffa3ce2

NiLuJe previously requested changes Feb 7, 2025

View reviewed changes

Comment thread frontend/ui/widget/doublespinwidget.lua Outdated

remove whitespace

c6c1986

Commodore64user requested a review from NiLuJe February 7, 2025 16:38

hius07 reviewed Feb 7, 2025

View reviewed changes

Commodore64user added 2 commits February 7, 2025 20:32

move vars declaration to init

12d2db1

and back to using self.

a6fe971

Commodore64user requested a review from poire-z February 7, 2025 23:34

poire-z reviewed Feb 7, 2025

View reviewed changes

move focus to ok button

a031a1f

Commodore64user added 4 commits February 11, 2025 02:34

Merge remote-tracking branch 'upstream/master' into double-spin

dbe635d

add hold_step option

c634858

update doc

4b04b85

correct event used by key_events

cdf363c

avoid crash

f2c61db

-something, instead of something * -1

bf295ed

Frenzie approved these changes Feb 17, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/master' into double-spin

74e39b1

Frenzie merged commit b758d7c into koreader:master Feb 27, 2025

Commodore64user deleted the double-spin branch February 27, 2025 19:51

Commodore64user mentioned this pull request Jun 14, 2025

[Presets] add new presets.lua module and dictionary presets #13774

Merged

0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026

[DoubleSpinWidget, SpinWidget] change values with page-turn buttons (k…

2cb5476

…oreader#13208)

Uh oh!

Uh oh!

Conversation

Commodore64user commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

what's new

enhancements to DoubleSpinWidget:

enhancements to SpinWidget:

others

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Commodore64user commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z commented Feb 4, 2025

Uh oh!

hius07 commented Feb 5, 2025

Uh oh!

Commodore64user commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hius07 commented Feb 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Commodore64user Feb 5, 2025 • 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!

Commodore64user commented Feb 5, 2025

Uh oh!

Frenzie commented Feb 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Commodore64user Feb 7, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hius07 Feb 8, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

hius07 commented Feb 8, 2025

Uh oh!

poire-z commented Feb 8, 2025

Uh oh!

Frenzie commented Feb 9, 2025

Uh oh!

NiLuJe commented Feb 10, 2025

Uh oh!

Commodore64user commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Feb 14, 2025

Uh oh!

Commodore64user commented Feb 14, 2025

Uh oh!

Frenzie commented Feb 14, 2025

Commodore64user commented Feb 4, 2025 •

edited

Loading

enhancements to `DoubleSpinWidget`:

enhancements to `SpinWidget`:

Commodore64user commented Feb 4, 2025 •

edited

Loading

Commodore64user commented Feb 5, 2025 •

edited

Loading

Commodore64user Feb 5, 2025 •

edited

Loading

Commodore64user Feb 7, 2025 •

edited

Loading

hius07 Feb 8, 2025 •

edited

Loading

Commodore64user commented Feb 12, 2025 •

edited

Loading