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

Touch zones: fix loss of overrides when re-registering a zone #5658

Merged
merged 1 commit into from Nov 30, 2019

Conversation

@poire-z
Copy link
Contributor

poire-z commented Nov 29, 2019

Happens with "Inverse reading order", which re-registers the Reader tap forward/backward zones and would prevent tap menu and bookmark from working.
Before this PR, when removing them, we would lose all the override= set on them by other touch zones (tap menu, bookmarks...), and so they were no more ensured after re-adding the zone.
So, make sure we don't lose that info. (Pinging @houqp for info, as DepGraph comes from you.)

This must be the proper fix for #5354, and revert a bit from #5423 (which made only the gestures set by ReaderGesture re-work - but not all others not managed by ReaderGesture like ReaderMenu Tap on top of screen). Pinging @yparitcher for info.


This change is Reviewable

@Frenzie Frenzie added this to the 2019.12 milestone Nov 29, 2019
@Frenzie Frenzie self-requested a review Nov 29, 2019
Copy link
Member

Frenzie left a comment

lgtm, just some stupid nitpicks

@@ -40,7 +40,19 @@ function DepGraph:addNode(node_key, deps)
end

function DepGraph:removeNode(node_key)
self.nodes[node_key] = nil
-- self.nodes[node_key] = nil

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

I don't see much point in leaving this line here?

@@ -98,4 +98,43 @@ describe("DepGraph module", function()
'readerhighlight_hold_release',
}, dg:serialize())
end)

it("should serialize complex graph and keep dependancies after remove and re-add", function()

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

I wouldn't have commented if that dependancies didn't stand out so much. :-P

Suggested change
it("should serialize complex graph and keep dependancies after remove and re-add", function()
it("should serialize complex graph and keep dependencies after removing and re-adding", function()

it("should serialize complex graph and keep dependancies after remove and re-add", function()
local dg = DepGraph:new{}
dg:addNode('tap_backward')

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

There doesn't seem to be any reason to use single quotes?

@@ -1577,11 +1577,6 @@ function ReaderGesture:onToggleReadingOrder()
local document_module = self.ui.document.info.has_pages and self.ui.paging or self.ui.rolling
document_module.inverse_reading_order = not document_module.inverse_reading_order
document_module:setupTouchZones()
-- Needed to reset the touch zone overrides

This comment has been minimized.

Copy link
@Frenzie

Frenzie Nov 30, 2019

Member

So this part was the ugly hack?

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 30, 2019

Author Contributor

Well, just a hack :) I don't think we ever did remove & re-add before, so that little bug in that trusted but forgotten bit of code was staying unnoticed.

@poire-z poire-z force-pushed the poire-z:fix_depgraph branch from dc4699e to 1d0053d Nov 30, 2019
Happens with "Inverse reading order", which re-registers
the Reader tap forward/backward zones and would prevent
tap menu and bookmark from working.
Before, when removing them, we would lose all the override=
set on them by other touch zones (tap menu, bookmarks...),
and so they were no more ensured after re-adding the zone.
So, make sure we don't lose that info.
@poire-z poire-z force-pushed the poire-z:fix_depgraph branch from 1d0053d to e12e43b Nov 30, 2019
Copy link
Member

Frenzie left a comment

lgtm, please merge when ready

@poire-z poire-z merged commit 3d19149 into koreader:master Nov 30, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z poire-z deleted the poire-z:fix_depgraph branch Nov 30, 2019
@yparitcher

This comment has been minimized.

Copy link
Contributor

yparitcher commented Nov 30, 2019

lgtm, i was fixing the symptom, i had not realized there was a deeper cause .

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