{"action":"created","comment":{"url":"https://api.github.com/repos/koreader/crengine/pulls/comments/171852279","pull_request_review_id":100780119,"id":171852279,"diff_hunk":"@@ -10516,6 +10516,15 @@ void ldomNode::recurseElementsDeepFirst( void (*pFun)( ldomNode * node ) )\n static void updateRendMethod( ldomNode * node )\n {\n     node->initNodeRendMethod();\n+    // Also clean up node previous positionnings (they were set while in\n+    // a previous page drawing phase), that could otherwise have negative\n+    // impact on the coming rendering (noticable with table elements).","path":"crengine/src/lvtinydom.cpp","position":6,"original_position":6,"commit_id":"acca2f296ae877f5d67a0e136955dde5e37eedca","original_commit_id":"acca2f296ae877f5d67a0e136955dde5e37eedca","user":{"login":"Frenzie","id":202757,"avatar_url":"https://avatars2.githubusercontent.com/u/202757?v=4","gravatar_id":"","url":"https://api.github.com/users/Frenzie","html_url":"https://github.com/Frenzie","followers_url":"https://api.github.com/users/Frenzie/followers","following_url":"https://api.github.com/users/Frenzie/following{/other_user}","gists_url":"https://api.github.com/users/Frenzie/gists{/gist_id}","starred_url":"https://api.github.com/users/Frenzie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Frenzie/subscriptions","organizations_url":"https://api.github.com/users/Frenzie/orgs","repos_url":"https://api.github.com/users/Frenzie/repos","events_url":"https://api.github.com/users/Frenzie/events{/privacy}","received_events_url":"https://api.github.com/users/Frenzie/received_events","type":"User","site_admin":false},"body":"noticeable","created_at":"2018-03-02T13:54:59Z","updated_at":"2018-03-02T13:54:59Z","html_url":"https://github.com/koreader/crengine/pull/102#discussion_r171852279","pull_request_url":"https://api.github.com/repos/koreader/crengine/pulls/102","author_association":"OWNER","_links":{"self":{"href":"https://api.github.com/repos/koreader/crengine/pulls/comments/171852279"},"html":{"href":"https://github.com/koreader/crengine/pull/102#discussion_r171852279"},"pull_request":{"href":"https://api.github.com/repos/koreader/crengine/pulls/102"}}},"pull_request":{"url":"https://api.github.com/repos/koreader/crengine/pulls/102","id":172499422,"html_url":"https://github.com/koreader/crengine/pull/102","diff_url":"https://github.com/koreader/crengine/pull/102.diff","patch_url":"https://github.com/koreader/crengine/pull/102.patch","issue_url":"https://api.github.com/repos/koreader/crengine/issues/102","number":102,"state":"open","locked":false,"title":"Fix some re rendering issues (mostly with tables)","user":{"login":"poire-z","id":24273478,"avatar_url":"https://avatars0.githubusercontent.com/u/24273478?v=4","gravatar_id":"","url":"https://api.github.com/users/poire-z","html_url":"https://github.com/poire-z","followers_url":"https://api.github.com/users/poire-z/followers","following_url":"https://api.github.com/users/poire-z/following{/other_user}","gists_url":"https://api.github.com/users/poire-z/gists{/gist_id}","starred_url":"https://api.github.com/users/poire-z/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/poire-z/subscriptions","organizations_url":"https://api.github.com/users/poire-z/orgs","repos_url":"https://api.github.com/users/poire-z/repos","events_url":"https://api.github.com/users/poire-z/events{/privacy}","received_events_url":"https://api.github.com/users/poire-z/received_events","type":"User","site_admin":false},"body":"I had been trying to figure out this bug every other month, and I'm glad I finally found a so small fix :)\r\n\r\nIt was quite noticable on Wikipedia articles saved as epub, that often have a multi-pages table (biography elements, numbers, various factual infos) at start of book.\r\nIt would display all fine on first opening, but when you change some layout option, or font sizes (or just toggle `Embedded fonts`, which should have zero impact on the rendering), the table would be messed up, with some big parts of it missing, and the number of pages would have decreased !\r\nAlso, it you then switch from `page` mode to `scroll` mode, it's all fine in scroll mode! and bad again when you switch back to `page` mode. (I also initially thought it was a problem with cache, but it was not).\r\n\r\nIt was that some part of the rendering process, which decides which y0->y1 (from the whole document 0->height) should fit on each page-mode page, got corrupted.\r\nWe would get (diff from initial rendering / second rendering) for a 6 pages book:\r\n```diff\r\n page         y0    y1      page_height\r\n    0:         0 .. 558     [558]\r\n    1:       558 .. 1125    [567]\r\n-   2:      1125 .. 1689    [564]\r\n-   3:      1689 .. 2254    [565]\r\n-   4:      2254 .. 2820    [566]\r\n-   5:      2820 .. 3097    [277]\r\n+   2:      1125 .. 1546    [421]\r\n+   3:      2165 .. 2728    [563]      (where did 1546 > 2165 go ?!)\r\n+   4:      2728 .. 3097    [369]\r\n```\r\nSo, some part of the (totally correct) document height would not be part of any page!\r\nScroll mode does not rely on this page splitting, and just cut slices of the whole document height according to scroll position.\r\n\r\nIt turned out that, in the positionning of table elements, some Y of the `<tr>`s, that were 0 on the first rendering, but were no more 0 on the second rendering, were wrongly added to the rendering positionning calculations.\r\nThis function (which is totally correct) does indeed add the parent `<tr>`, `<table>` ... to a `<td>` vertical positionning:\r\nhttps://github.com/koreader/crengine/blob/9cc32c831c3518eb7da7cd84f5a75646e354cebd/crengine/src/lvtinydom.cpp#L10456-L10461\r\nI first fixed it by just ignoring `<tr>` nodes there (as they are a kind of virtual element), but that was just a hack. So let's not touch it.\r\n\r\nThe `<tr>` Y were indeed set in the drawing phase that followed the first rendering (which was correct). Then, next renderings were using these Y, and pushing all table elements down, making them overflow. For some reason (not really clear to me), that would make the following text move up, and override the vertical part where the table should be (so, less number of pages in the book).\r\n\r\nFirst commit was my first clean attempt at fixing this.\r\n\r\nI then thought: why should just `<tr>` have this problem? Would be best to do it to all nodes.\r\nSo, the second commit of this PR remove the first, and do it in a generic way to all elements: this `updateRendMethod()` is called by `initNodeRendMethodRecursive()`, which is called only when cleaning things before a new rendering (it looks expensive, but it is really not).\r\n\r\nAnd with this second commit (not with the first), the problem described by @Eduardomb22  in https://github.com/koreader/koreader/issues/3623#issuecomment-359240654 with his book (table in §15.8.1.3 would have their cells all messed up when you change fonts size, layout, or just font hinting) is solved !\r\n\r\nSo, I guess this generic fix is the right thing to do.\r\n(We can squash these 2 commits, I made them two for reference and illustrative purpose only.)\r\n\r\n","created_at":"2018-03-02T13:11:28Z","updated_at":"2018-03-02T13:54:59Z","closed_at":null,"merged_at":null,"merge_commit_sha":"7150effcf32918fd54af1d8a60c2dd4cb2d5ce88","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/koreader/crengine/pulls/102/commits","review_comments_url":"https://api.github.com/repos/koreader/crengine/pulls/102/comments","review_comment_url":"https://api.github.com/repos/koreader/crengine/pulls/comments{/number}","comments_url":"https://api.github.com/repos/koreader/crengine/issues/102/comments","statuses_url":"https://api.github.com/repos/koreader/crengine/statuses/acca2f296ae877f5d67a0e136955dde5e37eedca","head":{"label":"poire-z:fix_table_re_rendering","ref":"fix_table_re_rendering","sha":"acca2f296ae877f5d67a0e136955dde5e37eedca","user":{"login":"poire-z","id":24273478,"avatar_url":"https://avatars0.githubusercontent.com/u/24273478?v=4","gravatar_id":"","url":"https://api.github.com/users/poire-z","html_url":"https://github.com/poire-z","followers_url":"https://api.github.com/users/poire-z/followers","following_url":"https://api.github.com/users/poire-z/following{/other_user}","gists_url":"https://api.github.com/users/poire-z/gists{/gist_id}","starred_url":"https://api.github.com/users/poire-z/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/poire-z/subscriptions","organizations_url":"https://api.github.com/users/poire-z/orgs","repos_url":"https://api.github.com/users/poire-z/repos","events_url":"https://api.github.com/users/poire-z/events{/privacy}","received_events_url":"https://api.github.com/users/poire-z/received_events","type":"User","site_admin":false},"repo":{"id":78963482,"name":"crengine","full_name":"poire-z/crengine","owner":{"login":"poire-z","id":24273478,"avatar_url":"https://avatars0.githubusercontent.com/u/24273478?v=4","gravatar_id":"","url":"https://api.github.com/users/poire-z","html_url":"https://github.com/poire-z","followers_url":"https://api.github.com/users/poire-z/followers","following_url":"https://api.github.com/users/poire-z/following{/other_user}","gists_url":"https://api.github.com/users/poire-z/gists{/gist_id}","starred_url":"https://api.github.com/users/poire-z/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/poire-z/subscriptions","organizations_url":"https://api.github.com/users/poire-z/orgs","repos_url":"https://api.github.com/users/poire-z/repos","events_url":"https://api.github.com/users/poire-z/events{/privacy}","received_events_url":"https://api.github.com/users/poire-z/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/poire-z/crengine","description":"Mirror of the official crengine (Cool Reader Engine) repository at sourceforge.net http://sourceforge.net/projects/crengine/","fork":true,"url":"https://api.github.com/repos/poire-z/crengine","forks_url":"https://api.github.com/repos/poire-z/crengine/forks","keys_url":"https://api.github.com/repos/poire-z/crengine/keys{/key_id}","collaborators_url":"https://api.github.com/repos/poire-z/crengine/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/poire-z/crengine/teams","hooks_url":"https://api.github.com/repos/poire-z/crengine/hooks","issue_events_url":"https://api.github.com/repos/poire-z/crengine/issues/events{/number}","events_url":"https://api.github.com/repos/poire-z/crengine/events","assignees_url":"https://api.github.com/repos/poire-z/crengine/assignees{/user}","branches_url":"https://api.github.com/repos/poire-z/crengine/branches{/branch}","tags_url":"https://api.github.com/repos/poire-z/crengine/tags","blobs_url":"https://api.github.com/repos/poire-z/crengine/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/poire-z/crengine/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/poire-z/crengine/git/refs{/sha}","trees_url":"https://api.github.com/repos/poire-z/crengine/git/trees{/sha}","statuses_url":"https://api.github.com/repos/poire-z/crengine/statuses/{sha}","languages_url":"https://api.github.com/repos/poire-z/crengine/languages","stargazers_url":"https://api.github.com/repos/poire-z/crengine/stargazers","contributors_url":"https://api.github.com/repos/poire-z/crengine/contributors","subscribers_url":"https://api.github.com/repos/poire-z/crengine/subscribers","subscription_url":"https://api.github.com/repos/poire-z/crengine/subscription","commits_url":"https://api.github.com/repos/poire-z/crengine/commits{/sha}","git_commits_url":"https://api.github.com/repos/poire-z/crengine/git/commits{/sha}","comments_url":"https://api.github.com/repos/poire-z/crengine/comments{/number}","issue_comment_url":"https://api.github.com/repos/poire-z/crengine/issues/comments{/number}","contents_url":"https://api.github.com/repos/poire-z/crengine/contents/{+path}","compare_url":"https://api.github.com/repos/poire-z/crengine/compare/{base}...{head}","merges_url":"https://api.github.com/repos/poire-z/crengine/merges","archive_url":"https://api.github.com/repos/poire-z/crengine/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/poire-z/crengine/downloads","issues_url":"https://api.github.com/repos/poire-z/crengine/issues{/number}","pulls_url":"https://api.github.com/repos/poire-z/crengine/pulls{/number}","milestones_url":"https://api.github.com/repos/poire-z/crengine/milestones{/number}","notifications_url":"https://api.github.com/repos/poire-z/crengine/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/poire-z/crengine/labels{/name}","releases_url":"https://api.github.com/repos/poire-z/crengine/releases{/id}","deployments_url":"https://api.github.com/repos/poire-z/crengine/deployments","created_at":"2017-01-14T20:00:48Z","updated_at":"2017-01-14T20:00:54Z","pushed_at":"2018-03-02T12:31:10Z","git_url":"git://github.com/poire-z/crengine.git","ssh_url":"git@github.com:poire-z/crengine.git","clone_url":"https://github.com/poire-z/crengine.git","svn_url":"https://github.com/poire-z/crengine","homepage":null,"size":18433,"stargazers_count":0,"watchers_count":0,"language":"C++","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":0,"license":null,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"koreader:master","ref":"master","sha":"9cc32c831c3518eb7da7cd84f5a75646e354cebd","user":{"login":"koreader","id":3957564,"avatar_url":"https://avatars0.githubusercontent.com/u/3957564?v=4","gravatar_id":"","url":"https://api.github.com/users/koreader","html_url":"https://github.com/koreader","followers_url":"https://api.github.com/users/koreader/followers","following_url":"https://api.github.com/users/koreader/following{/other_user}","gists_url":"https://api.github.com/users/koreader/gists{/gist_id}","starred_url":"https://api.github.com/users/koreader/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/koreader/subscriptions","organizations_url":"https://api.github.com/users/koreader/orgs","repos_url":"https://api.github.com/users/koreader/repos","events_url":"https://api.github.com/users/koreader/events{/privacy}","received_events_url":"https://api.github.com/users/koreader/received_events","type":"Organization","site_admin":false},"repo":{"id":16614401,"name":"crengine","full_name":"koreader/crengine","owner":{"login":"koreader","id":3957564,"avatar_url":"https://avatars0.githubusercontent.com/u/3957564?v=4","gravatar_id":"","url":"https://api.github.com/users/koreader","html_url":"https://github.com/koreader","followers_url":"https://api.github.com/users/koreader/followers","following_url":"https://api.github.com/users/koreader/following{/other_user}","gists_url":"https://api.github.com/users/koreader/gists{/gist_id}","starred_url":"https://api.github.com/users/koreader/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/koreader/subscriptions","organizations_url":"https://api.github.com/users/koreader/orgs","repos_url":"https://api.github.com/users/koreader/repos","events_url":"https://api.github.com/users/koreader/events{/privacy}","received_events_url":"https://api.github.com/users/koreader/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/koreader/crengine","description":"Mirror of the official crengine (Cool Reader Engine) repository at sourceforge.net http://sourceforge.net/projects/crengine/","fork":false,"url":"https://api.github.com/repos/koreader/crengine","forks_url":"https://api.github.com/repos/koreader/crengine/forks","keys_url":"https://api.github.com/repos/koreader/crengine/keys{/key_id}","collaborators_url":"https://api.github.com/repos/koreader/crengine/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/koreader/crengine/teams","hooks_url":"https://api.github.com/repos/koreader/crengine/hooks","issue_events_url":"https://api.github.com/repos/koreader/crengine/issues/events{/number}","events_url":"https://api.github.com/repos/koreader/crengine/events","assignees_url":"https://api.github.com/repos/koreader/crengine/assignees{/user}","branches_url":"https://api.github.com/repos/koreader/crengine/branches{/branch}","tags_url":"https://api.github.com/repos/koreader/crengine/tags","blobs_url":"https://api.github.com/repos/koreader/crengine/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/koreader/crengine/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/koreader/crengine/git/refs{/sha}","trees_url":"https://api.github.com/repos/koreader/crengine/git/trees{/sha}","statuses_url":"https://api.github.com/repos/koreader/crengine/statuses/{sha}","languages_url":"https://api.github.com/repos/koreader/crengine/languages","stargazers_url":"https://api.github.com/repos/koreader/crengine/stargazers","contributors_url":"https://api.github.com/repos/koreader/crengine/contributors","subscribers_url":"https://api.github.com/repos/koreader/crengine/subscribers","subscription_url":"https://api.github.com/repos/koreader/crengine/subscription","commits_url":"https://api.github.com/repos/koreader/crengine/commits{/sha}","git_commits_url":"https://api.github.com/repos/koreader/crengine/git/commits{/sha}","comments_url":"https://api.github.com/repos/koreader/crengine/comments{/number}","issue_comment_url":"https://api.github.com/repos/koreader/crengine/issues/comments{/number}","contents_url":"https://api.github.com/repos/koreader/crengine/contents/{+path}","compare_url":"https://api.github.com/repos/koreader/crengine/compare/{base}...{head}","merges_url":"https://api.github.com/repos/koreader/crengine/merges","archive_url":"https://api.github.com/repos/koreader/crengine/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/koreader/crengine/downloads","issues_url":"https://api.github.com/repos/koreader/crengine/issues{/number}","pulls_url":"https://api.github.com/repos/koreader/crengine/pulls{/number}","milestones_url":"https://api.github.com/repos/koreader/crengine/milestones{/number}","notifications_url":"https://api.github.com/repos/koreader/crengine/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/koreader/crengine/labels{/name}","releases_url":"https://api.github.com/repos/koreader/crengine/releases{/id}","deployments_url":"https://api.github.com/repos/koreader/crengine/deployments","created_at":"2014-02-07T12:47:11Z","updated_at":"2018-01-19T16:04:00Z","pushed_at":"2018-03-02T13:11:29Z","git_url":"git://github.com/koreader/crengine.git","ssh_url":"git@github.com:koreader/crengine.git","clone_url":"https://github.com/koreader/crengine.git","svn_url":"https://github.com/koreader/crengine","homepage":null,"size":18308,"stargazers_count":19,"watchers_count":19,"language":"C++","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":18,"mirror_url":null,"archived":false,"open_issues_count":5,"license":null,"forks":18,"open_issues":5,"watchers":19,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/koreader/crengine/pulls/102"},"html":{"href":"https://github.com/koreader/crengine/pull/102"},"issue":{"href":"https://api.github.com/repos/koreader/crengine/issues/102"},"comments":{"href":"https://api.github.com/repos/koreader/crengine/issues/102/comments"},"review_comments":{"href":"https://api.github.com/repos/koreader/crengine/pulls/102/comments"},"review_comment":{"href":"https://api.github.com/repos/koreader/crengine/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/koreader/crengine/pulls/102/commits"},"statuses":{"href":"https://api.github.com/repos/koreader/crengine/statuses/acca2f296ae877f5d67a0e136955dde5e37eedca"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"Frenzie"}}