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

Give more control over CRe margins #4945

Merged
merged 36 commits into from May 1, 2019

Conversation

Projects
None yet
4 participants
@NiLuJe
Copy link
Member

commented Apr 19, 2019

Without having to resort to weird custom defaults.

  • Split the current margins setting in three:

    • Horizontal margins (because you generally want those two to be balanced).
    • Top margin & Bottom margin (because you may want to tweak those separately to deal with quirky status bar/final line shenanigans).
  • Also, add a "Reclaim bar height from bottom margin" toggle to the status bar menu, to optionally make sure the status bar won't eat into the bottom margin.

  • Includes a free fix to diacritics popup refresh handling in the keyboard ;).

NiLuJe added some commits Apr 19, 2019

Never use partial in the keyboard.
It's REAGL, slow when queud with other types, which is pretty much
always with the keyboard ;).

Also, don't flash when showing/closing the diacritics popup.
Split vertical margin settings into top/bottom
To deal with statusbar & final line shenanigans in a less annoying
manner than unbalanced custom defaults...
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

For instance, because I'm a crazy person, I'm going to override all of those on my end to basically 0,1,2,3,4,5,6,... ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Speaking of: what's the use-case for the crazy high values on the top end of the scale?

Granted, I like very minimal margins, but those get run through scaleBySize, so, I'm slightly at a loss as to what the intended use could be ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

Something like an Onyx Boox Max2. By which I mean anything with a giant display.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

PS I also wonder what the point is of those contrast settings over 2 or so. That's about where they turn from "meh, not for me" to "GET IT AWAY FROM ME D:". ;-)

@Frenzie Frenzie added this to the 2019.05 milestone Apr 19, 2019

@Frenzie Frenzie added the UX label Apr 19, 2019

NiLuJe added some commits Apr 19, 2019

Keep using the same weird ublanaced defaults, because I can't be arsed
to update the crpaton of tests that rely on this
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

True ;o).

@Frenzie
Copy link
Member

left a comment

Looks fine to me. A little screenshot perhaps? :-)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

margins

(Not actually the default defaults ;p).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

For instance, because I'm a crazy person, I'm going to override all of those on my end to basically 0,1,2,3,4,5,6,... ;).

I'm not fond of having 2 additional progress bars just for margins (the more stuff there is, the less room there is to see their effect on the current book page).
But I see the point and the attempt to solve the other issue with the bottom margins and the feeling of missing a line at the bottom.
But this PR does not provide the solution to the default user :) only to the crazy person in the know who gonna edit their settings for that.

I'd rather see a single margin progress buttons + a fine tune progressbutton for bottom margins.
Or if really needed: a L/R margins progressbuttons + a T/B progressbuttons + a finetune bottom margin progressbutton.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

I initially started with a simple H/V split, and it turns out this doesn't help avoid unbalanced gaps between top/bottom (effective) margins.

So, yeah, one way or another, top AND bottom are better controlled separately.

A dynamic fine tune sounds like a great idea, I'm just not quite sure how to present that in terms of UI?

(And anything I can think of (i.e., something similar to the FL bar) wouldn't actually gain us any vertical space here).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

I also wonder what the point is of those contrast settings over 2 or so. That's about where they turn from "meh, not for me" to "GET IT AWAY FROM ME D:". ;-)

Might depend on the font and the eyesignt of the user... I use 4, and the max when i'm really tired.

Speaking of: what's the use-case for the crazy high values on the top end of the scale?

Not sure generally, but when I got my first progressive lenses, with a very narrow reading zone at the bottom, and being not used at all to move my head (only my eyes) when reading, i was happy to have koreader show me 25% margin on each side so I got a narrow readable zone with no blur on the sides...

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

Fair enough, although after a scaleBySize, I'm not quite sure we need to go that high ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

But speaking in terms of % of screen estate is interesting, and is indeed much less murky than this essentially somewhat meaningless unit we're currently using ;).

I feel like if we could eval' the file instead, it'd be much easier to
check than this insane game of whack'a mole... ;).
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

A dynamic fine tune sounds like a great idea, I'm just not quite sure how to present that in terms of UI?

Maybe:
L/R as you have it
T/B as you had it
Less B margin : 0 -1 -2 -3 -4 -5 -6 (whether scaleBySize or not, would be removed from the B margin set in the T/B button.
So one could decrease that B till he gets that missing line back.
I initially wrote "extra B margin", but I think it should have it decreased, to give the sense of having back that missing line.

(and this extra B would gain being available as a gesture for quick viewing its effect).

(And anything I can think of wouldn't actually gain us any vertical space here).

Might be a time to add a 6th bottom button for cre (which has currently 5, while PDF already has 6) :)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Any further comments?

local cfg_mt_idx = getmetatable(config).__index
local cfg_class
if cfg_mt_idx == DocSettings then
cfg_class = "local"

This comment has been minimized.

Copy link
@poire-z

poire-z Apr 30, 2019

Contributor

"book" rather than "local"?

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

I haven't played with it since the other day, but it looks allright.
May be wait for the coming stable release (named "Labor day" ? :) before merging, so we get one month to see how it does? (I'm still on my tedious/confused impression :)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Labor Day? Such sudden pressure, lol.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

I'd rather have it in the stable, to gather feedback from a potentially wider range of use-cases ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

It seems pretty safe to me. But then better put it in tonight or tomorrow so that it's had some testing by the time we reach the weekend. ;-)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Allright then.

@NiLuJe NiLuJe merged commit 888d359 into koreader:master May 1, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I have a couple flimsy reasons for leaving the tab in third position instead of second (as the crop tab is in KOpt). The first one is a smaller diff

Now that the smaller diff does not matter anymore, we can rediscuss that point :)

the second is because it depends (to some extent) on the DPI & line-height settings of the previous tab, so it felt sensible to leave them in that order, as you're more likely to fine-tune margins after having fine-tuned line-heights.

That's one use case, mine would probably be different and justify another order. (And if first-book-opening-usage order should justify tab orders, as you would probably choose the font size before lines height and page pargins, the font size tab should be second :)

But it looks odd the bottom tab icons are different between kopt and cre.
image

And there was some kind of logical order, from larger to smaller things, in kopt and in cre, that inverting 2nd and 3rd would bring back:
image

  • Screen (orientation)
  • Page (crop, page margins)
  • Lines (line heights, with zoom for elements/paragraphs margins in px and images)
  • Font size
  • Font smaller tweaks (kerning, hinting, contrast)
  • Last tab with other various thingies
@poire-z

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Small issue with the DogEar: when on a page with a bookmark, when reducing margins, it does not get its size adapted anymore, and it may end up eating into the text (previously fixed in #4081).
(It only has ReaderDogear:onSetPageMargins(), that we don't send anymore, now that we switched to SetPageHorizMargins SetPageTopMargin and SetPageBottomMargin.)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Still a bit awkward with the distinct top & bottom margins.
My use case (often used, always the same) is that I use font size 19 in the morning, and sometimes in the evening i need 20 or 21. Switching to 20 (with my other prefered unchanged settings) leaves a blank line at the bottom. That's what was discussed in #4876.
So, I want to get it back, and this PR allows it!
So, either I start with Top or Bottom. Starting with bottom is obviously better.

  1. Starting with Bottom: I reduce it, one or two steps, two is good with my use-case: one more line gained ! => 2 taps/rendering. But ok, it's no more centered.
  2. So, I reduce Top to get that centering back (I guess that if we are bothered with the missing/blank line at bottom, we are bothered by miscentering :) => 1 more tap/rendering.
  3. Now, I wouldn't need as much bottom margin, now that I gained some bit at the top (it may not matter much letting it as is, or it may on some pages , not sure): so, I increase again the Bottom margin => 1 more tap/rendering.
    So, that made 4 taps/renderings, which with some big books make take 10s to 60s each :|

Now that I get it with my usual use-case, I know I can only tap once on top and once on bottom, so I'll get 2 tap/renderings. Less time wasted. So, I may be somehow fine with this (unnatural) top & margin distinction.

I was going to quick&dirty hack that code to see how it would feel with the Top margin progress bar applied to both top & bottom margins (possibly hiding the Bottom margin progress bar if it felt alright).
But now that we have a specific tab for these margins settings, would it make sense to less quick&dirty hack it and have another toggle Lock T&B margins off | on (or a better name I can't find), so both progress bars would be kept sync'ed and both margins updated to the same value?
That would allow having the centering in 1 tap, and could be unlocked if some more precise tweaking is needed with some books or use-cases?

@poire-z

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

(OK, labor day or not, having a go at these last 2 points.)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Some refresh trick to exclude ConfigDialog from being painted on T&B margin change? And a 1 or 2s delay to trigger it's painting after that? :)

Had a quick try at that:

--- a/frontend/ui/data/creoptions.lua
+++ b/frontend/ui/data/creoptions.lua
@@ -102,2 +102,3 @@
                 event = "SyncPageTopBottomMargins",
+                delay_repaint = 2,
                 name_text_hold_callback = optionsutil.showValues,
@@ -139,2 +140,3 @@
                 },
+                delay_repaint = 2,
                 name_text_hold_callback = optionsutil.showValues,
@@ -171,2 +173,3 @@
                 },
+                delay_repaint = 2,
                 name_text_hold_callback = optionsutil.showValues,
--- a/frontend/ui/widget/configdialog.lua
+++ b/frontend/ui/widget/configdialog.lua
@@ -516,2 +516,3 @@ function ConfigOption:init()
                     events = self.options[c].events,
+                    delay_repaint = self.options[c].delay_repaint,
                     config = self.config,
@@ -546,3 +547,3 @@ function ConfigOption:init()
                             self.config:onConfigChoose(self.options[c].values, self.options[c].name,
-                                self.options[c].event, self.options[c].args, self.options[c].events, arg)
+                                self.options[c].event, self.options[c].args, self.options[c].events, arg, self.options[c].delay_repaint)
                         end
@@ -870,3 +871,3 @@ end

-function ConfigDialog:onConfigChoose(values, name, event, args, events, position)
+function ConfigDialog:onConfigChoose(values, name, event, args, events, position, delay_repaint)
     UIManager:tickAfterNext(function()
@@ -887,13 +888,22 @@ function ConfigDialog:onConfigChoose(values, name, event, args, events, position
         self:update()
-        if self.config_options.needs_redraw_on_change then
-            -- Some Kopt document event handlers just save their setting,
-            -- and need a full repaint for kopt to load these settings,
-            -- notice the change, and redraw the document
-            UIManager:setDirty("all", "partial")
+        local do_refresh = function()
+            self.skip_paint = nil
+            if self.config_options.needs_redraw_on_change then
+                -- Some Kopt document event handlers just save their setting,
+                -- and need a full repaint for kopt to load these settings,
+                -- notice the change, and redraw the document
+                UIManager:setDirty("all", "partial")
+            else
+                -- CreDocument event handlers do their own refresh:
+                -- we can just redraw our frame
+                UIManager:setDirty(self, function()
+                    return "ui", self.dialog_frame.dimen
+                end)
+            end
+        end
+        if delay_repaint then
+            UIManager:scheduleIn(delay_repaint, do_refresh)
+            self.skip_paint = true
         else
-            -- CreDocument event handlers do their own refresh:
-            -- we can just redraw our frame
-            UIManager:setDirty(self, function()
-                return "ui", self.dialog_frame.dimen
-            end)
+            do_refresh()
         end
--- a/frontend/ui/widget/container/inputcontainer.lua
+++ b/frontend/ui/widget/container/inputcontainer.lua
@@ -70,2 +70,5 @@ function InputContainer:paintTo(bb, x, y)
     end
+    if self.skip_paint then
+        return
+    end

--- a/frontend/ui/widget/toggleswitch.lua
+++ b/frontend/ui/widget/toggleswitch.lua
@@ -204,3 +204,3 @@ function ToggleSwitch:onTapSelect(arg, gev)
     self.config:onConfigChoose(self.values, self.name,
-                    self.event, self.args, self.events, self.position)
+                    self.event, self.args, self.events, self.position, self.delay_repaint)
     UIManager:setDirty(self.config, function()

It's quite ok (it just crashes if you switch the toggle too fast before the timeout), but I think it shouldn't be a timeout (to give the time to appreciate the result), and should probably be coupled with the InfoMessage showing the values, with an added line: "(Hit screen to dismiss)" so it shows the ConfigDialog again only then.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

and should probably be coupled with the InfoMessage showing the values, with an added line: "(Hit screen to dismiss)" so it shows the ConfigDialog again only then.

This patch would give this:

margins_tap_dismiss

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request May 2, 2019

Restore PAGE_MARGIN string
It's used by KOptConfig

Fix koreader#4992
Regression since koreader#4945

@NiLuJe NiLuJe referenced this pull request May 2, 2019

Merged

Restore PAGE_MARGIN string #4993

Frenzie added a commit that referenced this pull request May 2, 2019

[fix] Restore PAGE_MARGIN string (#4993)
It's used by KOptConfig

Fix #4992
Regression since #4945
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@poire-z : I have yet to try it, but I think I like it, and it doesn't look too gnarly ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

@poire-z : Yup, I like it in practice, too ;).

I kinda liked the timeout approach, though, but I don't know how feasible that would be here? (i.e., have a timeout, but one that you can cancel early by tapping?).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

I don't know how feasible that would be here

Just a matter of having dismiss_callback called here:

if self.timeout then
UIManager:scheduleIn(self.timeout, function() UIManager:close(self) end)
end

like it is done here:
function InfoMessage:onTapClose()
self.dismiss_callback()
UIManager:close(self)

(Possibly only if specified as an option, dunno if in general, a timeout should call a dismiss_callback, or if it should be called only if explicite dismiss with tap - I don't think we yet use these together.)

I kinda liked the timeout approach

I didn't :) Your 4 seconds were too near my mean re-action time :) So, either I was fast (3s!) and tapped on another progressbar button, and it didn't react as it was just dismissing the infomessage, and so I needed to tap another time. Or I was slow and it worked. Kinda upsetting :)

With this, we don't know how much time the user will want to appreciate the margins. Sometimes not much when they are bad - sometimes 10 seconds when they are fine. A timeout would show the bottom menu again, and he would have to tap again to dismiss it and see again the full page.
So, hard to guess a timeout value that won't upset some users. A always tap to dismiss gives a more deterministic experience I think.

(I'll make a PR of it after "not labor day but the sunday after" stable release :)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

Okay, let's forget about timeouts if you feel like 4s is too short then ;). Thanks for the pointer, though :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.