Dispatcher: Different default values than these in bottom menu

[mergen3107]

• KOReader version: 2024.04-191 (with minor non-related patches that don't affect this issue)
• Device: Kindle Scribe (FW 5.16.2)

Issue

I use Profiles for some fonts with six parameters to tweak:

• Font face (without SpinWidget)
• Font size (with SpinWidget, ~correct~ wrong default)
• Line spacing (with SpinWidget, wrong default)
• Weight (without SpinWidget)
• Kerning (without SpinWidget)
• Hinting (without SpinWidget)

The issue is that

Steps to reproduce

See pictures below. Line spacing from Profiles' setup:

Line spacing from the bottom menu:

crash.log (if applicable)

I have not crash.log, because I believe this issue is easily reproducible. I'll double-check in emulator as well. I believe this issue looks similar to wrong SpinWidget default value passed through as in https://github.com/koreader/koreader/pull/11999/commits/a90c10fdbccb86ff2fd1e8e8e694b540a9172cdc

I'll also wanna double-check all other parameters with SpinWidget.

[mergen3107]

What I realized is that these items in Profiles are actually coming from the Dispatcher itself. I'll see what's wrong with Dispatcher then...

[hius07]

https://github.com/koreader/koreader/blob/fa4654cdfa56e7452f6e6ae43d19cd65c105401c/frontend/ui/data/creoptions.lua#L382 https://github.com/koreader/koreader/blob/fa4654cdfa56e7452f6e6ae43d19cd65c105401c/defaults.lua#L141

[mergen3107]

Thanks @hius07 I searched with rip grep for Line Spacing and found these:

mergen3107@mateLTS22:~/koreader$ rg -C3 -g '!l10n/' "Line Spacing"
frontend/ui/data/creoptions.lua
361-            },
362-            {   -- ReaderFont
363-                name = "line_spacing",
364:                name_text = _("Line Spacing"),
365-                buttonprogress = true,
366-                values = {
367-                    G_defaults:readSetting("DCREREADER_CONFIG_LINE_SPACE_PERCENT_X_TINY"),

frontend/ui/data/css_tweaks.lua
404-                    title = _("Larger spacing between ruby lines"),
405-                    description = _([[
406-Increase line spacing of most text, so that lines keep an even spacing whether or not they include ruby.
407:Further small adjustments can be done with 'Line Spacing' in the bottom menu.]]),
408-                    css = [[p, li { line-height: 2 !important; }]],
409-                    -- no need for priority, this has higher specificity than lineheight_all_inherit below
410-                },

frontend/ui/data/koptoptions.lua
376-            },
377-            {
378-                name = "line_spacing",
379:                name_text = _("Line Spacing"),
380-                toggle = {C_("Line spacing", "small"), C_("Line spacing", "medium"), C_("Line spacing", "large")},
381-                values = {1.0, 1.2, 1.4},
382-                default_value = G_defaults:readSetting("DKOPTREADER_CONFIG_LINE_SPACING"),

I believe the new default setting that can be set in bottom menu should be overring it?

Something like:

default_value = self.BOTTOM_MENU_DEFAULT_LINE_SPACING or G_defaults:readSetting("DCREREADER_CONFIG_LINE_SPACE_PERCENT_MEDIUM"), 

where I need to find the actual variable name for self.BOTTOM_MENU_DEFAULT_LINE_SPACING.

[hius07]

Dispatcher does not read the user's "defaults", it shows the default "defaults", it's okay. Dispatcher does not pass "units" to SpinWidgets (so you do not see the % sign), it can be fixed I believe.

[mergen3107]

I did

default_value = G_reader_settings:readSetting("copt_line_spacing") or G_defaults:readSetting("DCREREADER_CONFIG_LINE_SPACE_PERCENT_MEDIUM"),

and it worked! Although, without % sign, but at least correct.

@hius07 Can I do PR or you have a better approach in mind?

[mergen3107]

Overall, I believe these DEFAULT_SETTINGS are legacy from crengine and they should be de-duplicated. I don't know how much work that is

[mergen3107]

• Font size (with SpinWidget, correct default)

OK, this was a false positive - it just happened to be the same as DCREREADER_CONFIG_DEFAULT_FONT_SIZE = 22, -- default font size in defaults.lua.

[mergen3107]

Ah, sorry. I take my fix above back: copt_ are not default settings, but rather just current ones. Where are the default ones are stored?

[hius07]

Overall, I believe these DEFAULT_SETTINGS are legacy from crengine and they should be de-duplicated.

Where do you propose to store the settings for the first run, after clean install?

[mergen3107]

In a file, but with proper notation with copt_ and kopt_ prefixes. By “legacy” I only mean that they now look different from current implementation of settings.

Maybe copt_default_settingname or copt_setting_name_default.

[mergen3107]

Simplest way (I think) to solve these mismatches without major SpinWidget overhauls is replacing global defaults with something like:

DCREREADER_CONFIG_LINE_SPACE_PERCENT_MEDIUM = G_reader_settings:readSetting("copt_line_spacing_default") or 100,

So, if this is the fresh build, the value will be 100 beacause the copt_line_spacing_default doesn't exist yet. However, it needs to be assigned after each Set as default SpinWidget action (which I still don't understand where it is saved).

[yparitcher]

https://github.com/koreader/koreader/blob/fa4654cdfa56e7452f6e6ae43d19cd65c105401c/frontend/ui/widget/configdialog.lua#L395-L396

[hius07]

@yparitcher, do you think we should introduce G_reader_settings in Dispatcher? (Just for the user's default values in SpinWidgets - font size, line spacing etc)

[yparitcher]

This line: https://github.com/koreader/koreader/blob/b5a822cf8b7dd3b584d2a6d78a14a557d5669c31/frontend/dispatcher.lua#L540 Should probably be:

                        settingsList[name].default = G_reader_settings:readSetting(name) or option.default_value

Untested, it may need:

                        default_name = prefix and name or 'copt_' .. option.name
                        settingsList[name].default = G_reader_settings:readSetting(default_name) or option.default_value

[hius07]

Generally speaking, I would remove "default value" from all SpinWidgets in Dispatcher (i.e. Gestures/Profiles action list). For the consistency reason: there are many other actions that have their default values (in the meaning of applying them to a new book), but we do not mark them in action list because only SpinWidget has the Default button. So we see the default values only for: CRE Font Size, Line Spacing, Top/Bottom Margin PDF H/V Overlap, Auto Straighten

[hius07]

But what definitely must be fixed: consistent min/max values and units in SpinWidgets in theDispatcher and in the ConfigDialog (bottom menu).

[hius07]

Is there anything to fix?

[mergen3107]

consistent min/max values and units in SpinWidgets in theDispatcher and in the ConfigDialog (bottom menu)

This?

[hius07]

I see. Let's look at the reflowable doc Line spacing. In the bottom menu we have a progress bar and a SpinWidget for it.

Progress bar has min 70 and max 130. https://github.com/koreader/koreader/blob/cb84dbd552698746b6f621817be96870e184f9c1/frontend/ui/data/creoptions.lua#L390-L391 https://github.com/koreader/koreader/blob/cb84dbd552698746b6f621817be96870e184f9c1/defaults.lua#L133-L134 SpinWidget has min 50 and max 200. https://github.com/koreader/koreader/blob/cb84dbd552698746b6f621817be96870e184f9c1/frontend/ui/data/creoptions.lua#L383-L384 Dispatcher (Gestures manager) takes min/max for its SpinWidget from args (i.e. from the progress bar values): https://github.com/koreader/koreader/blob/cb84dbd552698746b6f621817be96870e184f9c1/frontend/dispatcher.lua#L547-L550 So it gets 70/130.

We can fix it by making the consistent bottom menu: both progress bar and SpinWidget with the same min/max values.

[hius07]

Or, if it is good that SpinWidget in the bottom menu extends the progress bar limits, Dispatcher should take extended min/max too.

[NiLuJe]

I very very vaguely recall extending the SpinWidget's range on purpose because it was less unwieldy than dealing with old defaults (maybe not for this setting in particular, but, then again, it rings a bell ;)).

[Frenzie]

Or, if it is good that SpinWidget in the bottom menu extends the progress bar limits, Dispatcher should take extended min/max too.

This is the way I see it. The progressbar is for typical values while the spin widget includes values we might consider weirdly high or low.