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

Refactor devices, add cover size stuff #1

Merged
merged 15 commits into from
Nov 25, 2019
Merged

Refactor devices, add cover size stuff #1

merged 15 commits into from
Nov 25, 2019

Conversation

pgaskin
Copy link
Owner

@pgaskin pgaskin commented Oct 19, 2019

  • A lot more information (codenames, storage, etc)
  • Cover sizes
  • Cleaner code, easier to extend in the future
  • Codenames now match the internal layout (class -> family + optional secondary)
  • Based on firmware 4.18.13737

Also see https://gist.github.com/geek1011/613b34c23f026f7c39c50ee32f5e167e and shermp/Kobo-UNCaGED#16

cc @shermp, @NiLuJe, @davidfor

* A lot more information (codenames, storage, etc)
* Cover sizes
* Cleaner code, easier to extend in the future
* Codenames now match the internal layout (class -> family + optional secondary)
* Based on firmware 4.18.13737

Also see https://gist.github.com/geek1011/613b34c23f026f7c39c50ee32f5e167e and shermp/Kobo-UNCaGED#16
@pgaskin pgaskin added the enhancement New feature or request label Oct 19, 2019
N3_LIBRARY_FULL -> N3_FULL
Based on our (@geek1011, @NiLuJe) previous work for @shermp's Kobo-UNCaGED (shermp/Kobo-UNCaGED#17)
* Image part hashing based on @shermp's Kobo-UNCaGED code.
* Qt resizing algorithm re-implementation based on our (@NiLuJe, @geek1011) implementation for KU.
* Path/ImageID/ContentID conversion based on my seriesmeta code from kepubify.
Copy link
Owner Author

@pgaskin pgaskin left a comment

Choose a reason for hiding this comment

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

TODO:

  • Write more tests
  • Add more godoc comments
  • Update my tools which depend on this
  • Test cover generation (work will be in the covergen branch of kepubify)

Also, this is mostly ready for review now.

@pgaskin pgaskin self-assigned this Oct 19, 2019
@pgaskin pgaskin changed the title Refactor devices Refactor and improve devices, add cover size stuff Oct 19, 2019
@pgaskin pgaskin changed the title Refactor and improve devices, add cover size stuff Refactor devices, add cover size stuff Oct 19, 2019
pgaskin added a commit to pgaskin/kepubify that referenced this pull request Oct 19, 2019
@shermp
Copy link
Contributor

shermp commented Oct 19, 2019

Have you thought about having a generic "fallback" device, name etc with safe defaults such that new devices can still use the library, even if they aren't supported yet?

Also, this looks awesome. I'm inclined to switch to this library in Kobo-UNCaGED to avoid duplication of code.

Turns out it's actually what I originally put: N3_LIBRARY_FULL
@pgaskin
Copy link
Owner Author

pgaskin commented Oct 19, 2019

OK, I've tested the cover generation and some of the device detection as part of pgaskin/kepubify#44, and everything seems to work well.

kobo/device.go Show resolved Hide resolved
@shermp
Copy link
Contributor

shermp commented Oct 19, 2019

It's Qt4's QtHash, FWIW ;).

(c.f., https://github.com/NiLuJe/kfmon/blob/f19f8582e7b53d14b7a60cc5b8bc43bac1930b48/kfmon.c#L1096)

Ok, I was wondering how much time @shermp would have spent reversing this... 😃

I'll add a note about that for future reference.

Yeah, I lifted it straight from the Kobo driver in Calibre, which is where I originally saw it.

EDIT: Somewhere in our refactors, that little nugget of info was lost. My original comment was:

// genImagePath generates the directory structure used by
// kobo to store the cover image files.
// It has been ported from the implementation in the KoboTouch
// driver in Calibre
func genImageDirPath(imageID string) string {

kobo/device.go Outdated Show resolved Hide resolved
@pgaskin
Copy link
Owner Author

pgaskin commented Oct 29, 2019

Unless anyone has more suggestions/objections, I'm planning to merge this after I write tests for the cover stuff.

@pgaskin pgaskin merged commit 56ce949 into master Nov 25, 2019
@pgaskin
Copy link
Owner Author

pgaskin commented Nov 25, 2019

I've released this as v2.

@pgaskin
Copy link
Owner Author

pgaskin commented Nov 25, 2019

@shermp, are you planning on updating KU yourself?

@shermp
Copy link
Contributor

shermp commented Nov 25, 2019

I was thinking of pulling in koboutils as a library and using it instead.

No need to duplicate code/effort :)

@pgaskin
Copy link
Owner Author

pgaskin commented Nov 25, 2019

That was what I meant, but were you planning to implement it?

@shermp
Copy link
Contributor

shermp commented Nov 25, 2019

Yeah. Want to finalize the changes to UNCaGED first before updating KU.

@pgaskin
Copy link
Owner Author

pgaskin commented Nov 25, 2019

Sounds good.

@shermp
Copy link
Contributor

shermp commented Nov 25, 2019

Speaking of UNCaGED, would you be willing to have a look at the open PR at some point? I left it for a while, seeing as you were taking a break from Kobo stuff.

@pgaskin
Copy link
Owner Author

pgaskin commented Nov 25, 2019

Yep, will do today or tomorrow. I'll be taking another break afterwards until the next firmware release.

@shermp
Copy link
Contributor

shermp commented Nov 25, 2019

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants