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

Auto crop fix #11194

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Auto crop fix #11194

merged 2 commits into from
Dec 4, 2023

Conversation

hugleo
Copy link
Contributor

@hugleo hugleo commented Dec 3, 2023

After some tests with the said document from #970 it seems like that the visual defect occurs for only semi-auto and manual crop modes.

I've removed auto-crop from the rule so #4106 can be fixed when using auto-crop.


This change is Reviewable

After some tests with the said document from koreader#970 it seems like that the visual defect occurs for only semi-auto and manual crop modes.

I've removed auto-crop from the rule so koreader#4106 can be fixed when using auto-crop.
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, might be worth adding some parentheses for clarity even though it makes no functional difference?

@Frenzie Frenzie added this to the 2023.12 milestone Dec 3, 2023
@hugleo
Copy link
Contributor Author

hugleo commented Dec 3, 2023

If someone wants to dig more on this later I'm sending two test files, same content and dimensions.
test_document.pdf
test_document.djvu (Similar behavior to #970 said file)

If someone wants to know the problem that #970 fixed, follow:
The tests are with 970 reverted back.
Effect of executing self.view:onBBoxUpdate(ubbox_dimen) for all cases.
For the tests here we open the document with all settings empty for each test, apply the crop mode on page 1 and move to page 2. The images show a bit of page 1 and the defect on page 2.

for test_document.pdf
semi-auto crop
Looks normal
semi-auto-pdf

manual crop
Visual defect occurs (looks more or less)
manual-pdf

for test_document.djvu
semi-auto crop
Visual defect occurs
semi-auto-djvu

manual crop
Visual defect occurs
manual-djvu

Interesting for pdf we got the visual defect only for manual crop. Because djvu fails for semi-auto and manual crop modes, I'm keeping #970 for both. So #4106 will look fine for auto-crop but will still fail for semi-auto crop, unless we choose to throw the semi-auto crop problem on djvu side.

tests.zip

@@ -527,7 +527,7 @@ function ReaderZooming:getZoom(pageno)
local ubbox_dimen = self.ui.document:getUsedBBoxDimensions(pageno, 1)
-- if bbox is larger than the native page dimension render the full page
-- See discussion in koreader/koreader#970.
if ubbox_dimen.w <= page_size.w and ubbox_dimen.h <= page_size.h then
if (ubbox_dimen.w <= page_size.w and ubbox_dimen.h <= page_size.h) or (self.ui.document.configurable.trim_page == 1) then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant the first one really, on the second it looks weird. :-P

But pinging @poire-z for some thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind the parens around the second one. It looks balanced, and may save 500ms of brain processing power while reading :)

@Frenzie Frenzie merged commit f990937 into koreader:master Dec 4, 2023
3 checks passed
@hugleo hugleo mentioned this pull request Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants