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

Pass arbitrary DjVu text annotations to frontend #909

Merged
merged 2 commits into from
Apr 24, 2019

Conversation

xelxebar
Copy link
Contributor

@xelxebar xelxebar commented Apr 24, 2019

Overview

Partially addresses koreader/koreader#2943.

This just transparently translates the annotation tree into a Lua table. The frontend needs to be prepared to handle this more flexible structure. Fortunately, almost all DjVu files in practice adhere to the rigid page -> line -> word hierarchy, so this patch should transparently work without any noticeable difference in practice.

Comments

The final product here is my original implementation, which departs from the prevailing style of djvu.c, so I made an attempt at orthogonalizing style choices into the second commit.

Currently, djvu.c has a lot of magic numbers and repeated code, so my goal was to give these understandable names and make them reusable. Arguably, this should be it's own pull request, and be carried out file-wide, but I figured that the comparison here might help review, so am including it.

@xelxebar xelxebar force-pushed the bugfix/2943-djvu-txt-zone-hierarchy branch 2 times, most recently from d8fab33 to 85120d4 Compare April 24, 2019 08:42
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 for a rebase

@Frenzie
Copy link
Member

Frenzie commented Apr 24, 2019

Btw, in principle the coding style of the project is with 4 spaces instead of tabs, but I suppose we should just keep this file in tabs.

But part of me wonders if we should migrate "organically". ;-)

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.

This is much clearer btw, thanks!

@xelxebar
Copy link
Contributor Author

@Frenzie
Roger on the spaces vs. tabs. I have half a mind to do some housecleaning on djvu.c anyway, and if I do that might be the time to switch. Or did you mean migrate everything else the other way? lol

Thanks for the props! I put a Doxygen-style comment header on lua_settable_djvu_anno, but is there any project convention on this too? I haven't noticed any from my little bit of poking around.

@xelxebar
Copy link
Contributor Author

Strange that spec/unit/md5_spec.lua would be failing on this.

@Frenzie
Copy link
Member

Frenzie commented Apr 24, 2019

Thanks for the props! I put a Doxygen-style comment header on lua_settable_djvu_anno, but is there any project convention on this too? I haven't noticed any from my little bit of poking around.

The project is mainly Lua. We don't really have any formal C conventions. However, if it doesn't matter for Doxygen then it might be best to stick closer to the luadoc style for project-wide consistency, in this case with an @ instead of a \: https://github.com/koreader/koreader/blob/c0ba6ae48ea3ee9b693448b43e2c5f30db5991b7/frontend/util.lua#L70-L77

Strange that spec/unit/md5_spec.lua would be failing on this.

It's not related to this PR. It's some issue with the Docker Clang container I made, but I can't reproduce it locally (in the exact same Docker container) and the CircleCI SSH login didn't work for me yesterday.

I have no idea why it's failing.

B. Wilson added 2 commits April 24, 2019 18:42
Partially addresses koreader/koreader#2943. This just transparently
translates the annotation tree into a Lua table. The frontend needs to
be prepared to handle this more flexible structure.

Fortunately, almost all DjVu files in practice adhere to the rigid `page
-> line -> word` hierarchy, so this patch should transparently work
without any noticeable difference in practice.
The code in `djvu.c` contains a lot of repetition and magic
numbers/strings. This makes an inital attempt at factoring some of that
into reusable, self-documenting units. The `lua_settable_djvu_anno` is
then refactored to use these new definitions.
@xelxebar xelxebar force-pushed the bugfix/2943-djvu-txt-zone-hierarchy branch from 85120d4 to bfc172e Compare April 24, 2019 09:42
@xelxebar
Copy link
Contributor Author

xelxebar commented Apr 24, 2019

Pushed change to comment. Doxygen accepts @ as well.
Thanks for the quick feedback.

@Frenzie Frenzie merged commit 36fca21 into koreader:master Apr 24, 2019
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request May 1, 2019
Mainly for the switch to the C BB by default on Kobo/Kindle/PB

But also includes:

CI tweaks: koreader/koreader-base#907
           koreader/koreader-base#911
DjVu tweaks: koreader/koreader-base#909
SDL2 tweaks: koreader/koreader-base#910
NiLuJe added a commit to koreader/koreader that referenced this pull request May 1, 2019
Mainly for the switch to the C BB by default on Kobo/Kindle/PB

But also includes:

CI tweaks: koreader/koreader-base#907
           koreader/koreader-base#911
DjVu tweaks: koreader/koreader-base#909
SDL2 tweaks: koreader/koreader-base#910
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
Mainly for the switch to the C BB by default on Kobo/Kindle/PB

But also includes:

CI tweaks: koreader/koreader-base#907
           koreader/koreader-base#911
DjVu tweaks: koreader/koreader-base#909
SDL2 tweaks: koreader/koreader-base#910
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants