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

Page overlap menu (cre): set nb of overlap lines #4538

Merged
merged 1 commit into from Feb 3, 2019

Conversation

Projects
None yet
3 participants
@poire-z
Copy link
Contributor

poire-z commented Feb 1, 2019

Make the existing setting (introduced in #3870) tunable with a menu item.
Also make the Page overlap and Highlight menus (with pdf or cre) items grayed out when disabled, and use a checkbox instead of the strange Enable/Disable.

image
image

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Feb 1, 2019

That overlap thing is still messy (both in pdf and epub, there are a few issues opened), not intending to fix them (and I don't use them).

And on epub, the Gray out option is usable, but the Arrow one is messy, and show some black square at top of screen, because of some bug in AlphaContainer (background_bb_x/y are never set, so the black square at 0/0, but it's a bit unclear how/why that background stuff...) so not touching any more stuff.

image

Just pasting the state of my attempt in case I (or someone else) need to revisit that (that patch that would break Arrow with PDF):

--- a/frontend/apps/reader/modules/readerview.lua
+++ b/frontend/apps/reader/modules/readerview.lua
@@ -185,8 +185,17 @@ function ReaderView:paintTo(bb, x, y)
                 self.dim_area.w, self.dim_area.h
             )
         elseif self.page_overlap_style == "arrow" then
-            self.arrow:paintTo(bb, 0, self.dim_area.h)
-        end
+            -- This is wrong with PDF, where the dim area show parts
+            -- of previous page, and we may get a huge dim area if
+            -- the page nearly fit the screen
+            if self.dim_area.y < Screen:getSize().h / 2 then
+                -- dim area at top: show arrow at its bottom
+                self.arrow:paintTo(bb, 0, self.dim_area.h)
+            else
+                -- dim area at bottom: show arrow at its top
+                self.arrow:paintTo(bb, 0, self.dim_area.y)
+            end
+         end
     end
     -- draw saved highlight
     if self.highlight_visible then
--- a/frontend/ui/widget/container/alphacontainer.lua
+++ b/frontend/ui/widget/container/alphacontainer.lua
@@ -33,36 +33,36 @@ local AlphaContainer = WidgetContainer:new{
 function AlphaContainer:paintTo(bb, x, y)
     local contentSize = self[1]:getSize()
     local private_bb = self.private_bb
+    require("logger").warn(">>>>>>>>>>>>>>>>>", x, y, background_bb_x, background_bb_y)

-    if self.background_bb then
+    if self.background_bb and self.background_bb_x == x and self.background_bb_y == y then
         -- we have a saved copy of what was below our paint area
         -- we restore this first
         bb:blitFrom(self.background_bb, self.background_bb_x, self.background_bb_y)
+        require("logger").warn("<<<<<<<<<<<<<<<<", self.background_bb_x, self.background_bb_y)
     end

-    if not private_bb
-    or private_bb:getWidth() ~= contentSize.w
-    or private_bb:getHeight() ~= contentSize.h
-    then
+    if not private_bb or private_bb:getWidth() ~= contentSize.w
+                      or private_bb:getHeight() ~= contentSize.h then
         if private_bb then
             private_bb:free() -- free the one we're going to replace
         end
         -- create private blitbuffer for our child widget to paint to
         private_bb = BlitBuffer.new(contentSize.w, contentSize.h, bb:getType())
         self.private_bb = private_bb
+    end

         -- save what is below our painting area
-        if not self.background_bb
-        or self.background_bb:getWidth() ~= contentSize.w
-        or self.background_bb:getHeight() ~= contentSize.h
-        then
-            if self.background_bb then
-                self.background_bb:free() -- free the one we're going to replace
-            end
-            self.background_bb = BlitBuffer.new(contentSize.w, contentSize.h, bb:getType())
+    if not self.background_bb or self.background_bb:getWidth() ~= contentSize.w
+                              or self.background_bb:getHeight() ~= contentSize.h then
+        if self.background_bb then
+            self.background_bb:free() -- free the one we're going to replace
         end
-        self.background_bb:blitFrom(bb, 0, 0, x, y)
+        self.background_bb = BlitBuffer.new(contentSize.w, contentSize.h, bb:getType())
     end
+    self.background_bb:blitFrom(bb, 0, 0, x, y)
+    self.background_bb_x = x
+    self.background_bb_y = y

     -- now have our childs paint to the private blitbuffer
     -- TODO: should we clean before painting?
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 1, 2019

That overlap thing is still messy (both in pdf and epub, there are a few issues opened), not intending to fix them (and I don't use them).

I use them (without modifying any of it) but I've never seen a problem. That being said, I don't use it in EPUB.

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Feb 1, 2019

I'm still confused as to why the option itself (as in, its menu entry to toggle it) is (sometimes) hidden behind DSHOWOVERLAP

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Feb 1, 2019

For PDF, these are the conditionals:

menu_items.page_overlap = {
text = _("Page overlap"),
enabled_func = function()
return not self.view.page_scroll and self.zoom_mode ~= "page"
and not self.zoom_mode:find("height")
end,
sub_item_table = page_overlap_menu,
}

So, you need to not be in scroll mode (unlike cre where you need to be in scroll mode :) and in a zoom mode that does not fit to height (so there's some height not shown that enable moving up/down the page, and so some other height previously already shown that can be shown grayed out)...

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 1, 2019

Oh, I guess I mean I don't use it then? What I mean is I normally use PDF/DjVu in continuous mode (scroll mode is a misnomer imo, but that aside).

title_text = _("Set overlapped lines"),
text = _([[
When page overlap is enabled, some lines from the previous pages are shown on the next page.
You can set how many lines are to be shown.]]),

This comment has been minimized.

@Frenzie

Frenzie Feb 1, 2019

Member
Suggested change
You can set how many lines are to be shown.]]),
You can set how many lines are shown.]]),
ok_text = _("Set"),
title_text = _("Set overlapped lines"),
text = _([[
When page overlap is enabled, some lines from the previous pages are shown on the next page.

This comment has been minimized.

@Frenzie

Frenzie Feb 1, 2019

Member
Suggested change
When page overlap is enabled, some lines from the previous pages are shown on the next page.
When page overlap is enabled, some lines from the previous pages will be displayed on the next page.

Just avoiding "shown" twice in a row. ;-)

end,
keep_menu_open = true,
help_text = _([[
When page overlap is enabled, some lines from the previous pages are shown on the next page.

This comment has been minimized.

@Frenzie

Frenzie Feb 1, 2019

Member

Should it be a local variable a little higher up, since the text is exactly the same?

table.insert(page_overlap_menu, menu_entry)
end
menu_items.page_overlap = {
text = _("Page overlap"),
enabled_func = function() return self.view.view_mode ~= "page" end,
help_text = _([[When page overlap is enabled, some lines from the previous pages are shown on the next page.]]),

This comment has been minimized.

@Frenzie

Frenzie Feb 1, 2019

Member
Suggested change
help_text = _([[When page overlap is enabled, some lines from the previous pages are shown on the next page.]]),
help_text = _([[When page overlap is enabled, some lines from the previous pages will be shown on the next page.]]),

Just making the suggestions consistent.

@poire-z poire-z force-pushed the poire-z:overlap_menu branch from 519e410 to 63e5454 Feb 1, 2019

Page overlap menu (cre): set nb of overlap lines
Make this existing setting tunable with a menu item.
Also make the Page overlap and Highlight menu items grayed out
when disabled.

@poire-z poire-z force-pushed the poire-z:overlap_menu branch from 63e5454 to 80a1849 Feb 1, 2019

@Frenzie Frenzie added the UX label Feb 1, 2019

@poire-z poire-z merged commit 895589d into koreader:master Feb 3, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:overlap_menu branch Feb 3, 2019

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.