PDF contrast: incorrect set by a gesture

[hius07]

Closes #10795.

---

This change is 

[hius07]

(1) The item is called "Contrast" in the bottom menu and in the Taps and gestures menu. Notification shows "Font gamma set to" message. Should it be changed to "Contrast"?

(2) Do we need the more_options ⋮ item in the bottom menu next to the progress bar?

(3) If not (2), do we need the SpinWidget in the Taps and gestures actions menu?
Or the fixed set of values {0.8, 1.0, 1.5, 2.0, 4.0, 6.0, 10.0, 50.0} is enough?

[yparitcher]

1. Yes

[poire-z]

Notification shows "Font gamma set to" message. Should it be changed to "Contrast"?

Is that just about PDF ?
In CRE, this contrast applies only to text/font - not to images. So, mentionning "font" may be fine.
I guess "font gamma" is quite strange, dunno how many people know what "gamma" means (i don't :) and I can't even find anything via Google...)

And I think this contrast/gamma in CRE should not apply to images. I sometimes miss another setting that would apply to images that could make grey text in images more readable :) I'm usually annoyed when I need more contrast in PDF for text, and it make all images nearly black... but I guess we can't distinguish text from images in PDF.

[hius07]

Yes, this PR is about pdf only.

koreader/frontend/apps/reader/modules/readerview.lua

Line 997 in ac143ca

Notification:notify(T(_("Font gamma set to: %1."), gamma))

cre shows the same:

koreader/frontend/apps/reader/modules/readerfont.lua

Line 350 in ac143ca

Notification:notify(T(_("Font gamma set to: %1."), gamma_level))

[hius07]

(2) Do we need the more_options ⋮ item in the bottom menu next to the progress bar?

For cre we have the vice versa inconsistency: the bottom menu has more_options ⋮ item, but Taps and gestures actions menu shows Contrast preset values, not the SpinWidget.

[NiLuJe]

I would very much keep those comments around ;).

[NiLuJe]

With an addendum that the div was moved to KoptInterface ;).

[hius07]

koreader/frontend/ui/data/koptoptions.lua

Line 375 in 0db7509

return optionsutil.enableIfEquals(configurable, "text_wrap", 1)

What is optimized by calling a function instead of return configurable.text_wrap == 1?

[poire-z]

What is optimized by calling a function instead of return configurable.text_wrap == 1?

Dunno, maybe nothing - it seems a remnant from some old PR where the author decided to have other such functions to maybe make things more readable? #1455.

[Frenzie]

What is optimized by calling a function instead of return configurable.text_wrap == 1?

I think the answer to most such questions would be clarity and/or maintainability? :-)

[hius07]

So, let's keep it?
Is it a clarity - to change == with a word "equal"?
Easy to maintain? If some day the reflow mode is detected in other way, it will definitely require to change all the calls, not only the function itself.

[poire-z]

Do as you prefer :)
(My habit is to not change what I don't need to - but as you have become the boss of the bottom menu hell, you're free to do as you like :))

[Frenzie]

Easy to maintain?

I basically mean greppability. It looks to me like optionsutil.enableIfEquals has a very high greppability factor, which normally counts as a positive in my book.

NB I'm not saying I'd have written it the same way or that I'm opposed to changing it; I barely know what the context is here. It's been at least a couple of years since I last dove into this code. I'm just speaking generically about code/maintenance patterns.

[hius07]

Update: make pdf Contrast consistent to cre Contrast (⋮ item next to the progress bar in the bottom menu; no SpinWidget in Taps and gestures menu, just eight preset values).

[hius07]

I believe the bug should be fixed.

[poire-z]

Looks ok to me. @yparitcher : ok for you ?
Please juste update CURRENT_MIGRATION_DATE = 20230810 to today :)

[yparitcher]

Does this work if the user has a gesture/profile saved with the old values?
Also the migration only fixes the global setting, what if i open a old document?
Not at my computer and I don't remember if we store the value or the arg or label.

[hius07]

Old setting will become new setting

koreader/frontend/apps/reader/modules/readerview.lua

Lines 863 to 866 in 5f336a7

	if config:has("gamma") then -- old doc contrast setting
	config:saveSetting("kopt_contrast", config:readSetting("gamma"))
	config:delSetting("gamma")
	end

And old calls

koreader/frontend/apps/reader/modules/readerkoptlistener.lua

Line 27 in ed2ea68

self.ui:handleEvent(Event:new("GammaUpdate", 1/self.document.configurable.contrast))

koreader/frontend/document/koptinterface.lua

Line 147 in ed2ea68

kc:setContrast(doc.configurable.contrast)

will become new calls

koreader/frontend/apps/reader/modules/readerkoptlistener.lua

Line 24 in 5f336a7

self.ui:handleEvent(Event:new("GammaUpdate", self.document.configurable.contrast))

koreader/frontend/document/koptinterface.lua

Line 148 in 5f336a7

kc:setContrast(1 / doc.configurable.contrast)

Following #1299 (comment), instead of saving 2 values for non-reflow and reflow modes (gamma and 1/gamma), we now save only one value, and in reflow mode use 1/value as argument to setContrast.

[yparitcher]

Does the config value need to be updated config:saveSetting("kopt_contrast", 1/ config:readSetting("gamma"))?

Also what about gestures that store call the gamma event directly?

[hius07]

Does the config value need to be updated

No, because option values are changed too.
Before:

koreader/frontend/ui/data/koptoptions.lua

Line 486 in ed2ea68

values = {1/0.8, 1/1.0, 1/1.5, 1/2.0, 1/4.0, 1/6.0, 1/10.0, 1/50.0},

After:

koreader/frontend/ui/data/koptoptions.lua

Line 486 in 5f336a7

values = {0.8, 1.0, 1.5, 2.0, 4.0, 6.0, 10.0, 50.0},

[yparitcher]

I see now.
Sorry for being slow today :)