Code improvements

[pgaskin]

• Decouple Kobo device info from KoboUncaged code
• Make Go code more idiomatic
• Improve handling of JSON
• Other cleanup stuff

[NiLuJe]

Note that FBInk also exposes the device name & id, as well as the screen's resolution, so that may be another way to go about it ;).

The ID is the same as the one found in the version file, and the name is roughly the common name of the device (instead of its board codename).

As that name is more of a "display" name, I'll probably end up storing the codename somewhere, too, as that's potentially less clunky to handle than the raw device ID (and makes it easier to avoid stuff falling through the cracks, like the few devices w/ multiple board revisions or a 32GB version).

[pgaskin]

I'm finished with this for now. When reading the changes, I suggest looking at each commit individually, as it'll be way easier to understand that way.

@NiLuJe or @shermp, can one of you test this before merging? I don't use calibre myself, and I need a second pair of eyes to make sure I didn't mix up some of the filenames when refactoring (the variable names are quite similar).

[shermp]

Note that FBInk also exposes the device name & id, as well as the screen's resolution, so that may be another way to go about it ;).

The ID is the same as the one found in the version file, and the name is roughly the common name of the device (instead of its board codename).

As that name is more of a "display" name, I'll probably end up storing the codename somewhere, too, as that's potentially less clunky to handle than the raw device ID (and makes it easier to avoid stuff falling through the cracks, like the few devices w/ multiple board revisions or a 32GB version).

Just a note on the model, and why I haven't used FBInk for that. I basically wanted to be able to run the code through a debugger, and there is currently no ARM debugger available for Go. Therefore I stuck the FBInk stuff behind an interface so I didn't have to build it, because go-fbink-v2 is only set up to build on ARM.

So the "print stuff to user" code is basically abstracted away from the rest of the code.

I'm open to using a different method to identify Kobo's. Main usage is to set appropriate cover dimensions, and I should probably be enforcing a minimum FW version as well.

[NiLuJe]

Nothing wrong with the method, really ;).

That was just a suggestion to avoid having to duplicate a copy of that logic, as it's potentially annoying to maintain in the long run ;).

[shermp]

Thank you SO much for helping with this @geek1011

A second pair of eyes on the code is a great help. Also shows my inexperience with Go, but hey we all have to start somewhere...

I'm definitely a bit embarrassed about that thumbnail image generation/saving code. It truly was a mess wasn't it?

[pgaskin]

No problem! I started out similarly a few years ago (I could never figure out the right way to use errors, I kept implementing concurrency wrong, and I often overused the make function). I'm still learning more, especially around layout for webapps, even now.

I agree that the thumbnail code was a mess. It was almost like a combination of a bunch of other languages (trying to use untyped arrays by casting, error handling layout like Java, etc). :smiley:

Also, you may want to try github.com/nfnt/resize for the resizing. I found it a bit faster (~66ms per image) when using it for BookBrowser (but some of my covers looked unusual with it, so I decided to use bicubic). Using lancos2 instead of bicubic makes it a bit faster. If I have time, I'll do some benchmarks of the different libs on my Kobo.

[NiLuJe]

Huh, a two lobed Lanczos being faster than bicubic is surprising ;) (I think? :D). But good news, as it should ultimately be a tiny bit sharper, too.

Note that Qt's own scaling isn't on the sharp side of things either, so, ultimately, it doesn't matter all that much, as that's both what Nickel & Calibre use ;).

[pgaskin]

Yep. It's probably faster due to implementation-specific inefficiencies (lanczos should be slower, like you mentioned) because the Go image-processing libs aren't quite mature yet.

[shermp]

I agree that the thumbnail code was a mess. It was almost like a combination of a bunch of other languages (trying to use untyped arrays by casting, error handling layout like Java, etc). 😃

Part of my problem is I'm not a full time developer, and up until a couple of years ago, I tended to actively avoid programming. It's really only in the past ~12 months that I've really started to get more serious with it. And during that time, I've jumped around using Go, C & Python. All of this probably bleeds through my coding style.

Also, you may want to try github.com/nfnt/resize for the resizing. I found it a bit faster (~66ms per image) when using it for BookBrowser (but some of my covers looked unusual with it, so I decided to use bicubic). Using lancos2 instead of bicubic makes it a bit faster. If I have time, I'll do some benchmarks of the different libs on my Kobo.

I looked into this, but was a bit put off by the fact that the author appears to have abandoned the project, going by the readme.

Also, I don't think benchmarking image code on non-ARM platforms is of much benefit, especially if they are using any SIMD codepaths.

[pgaskin]

Part of my problem is I'm not a full time developer, and up until a couple of years ago, I tended to actively avoid programming. It's really only in the past ~12 months that I've really started to get more serious with it. And during that time, I've jumped around using Go, C & Python. All of this probably bleeds through my coding style.

Yep.

I looked into this, but was a bit put off by the fact that the author appears to have abandoned the project, going by the readme.

It works pretty well as-is, and I've had less issues with it than other libraries.

Also, I don't think benchmarking image code on non-ARM platforms is of much benefit, especially if they are using any SIMD codepaths.

I'm going to be running the benchmarks on my Kobo.

[shermp]

I'm going to be running the benchmarks on my Kobo.

That works :D

[shermp]

No problem! I started out similarly a few years ago (I could never figure out the right way to use errors, I kept implementing concurrency wrong, and I often overused the make function). I'm still learning more, especially around layout for webapps, even now.

Oh errors... tries to hide the upstream UNCaGED from @geek1011

Yeah, upstream error handling needs work (or an overhaul). You may have seen the couple of instances where I've compared an error string here winces

[pgaskin]

Oh errors... tries to hide the upstream UNCaGED from @geek1011

Oh, I saw that earlier :rofl:. I was seeing if it would be feasible to try and implement a calibre-free UNCaGED server (I might try it when I get a chance).

Yeah, upstream error handling needs work (or an overhaul). You may have seen the couple of instances where I've compared an error string here winces

I noticed. I'm still guilty of that sometimes ... but I've been playing around with different options (my favourite is currently a custom error type with a field for a message, a wrapped error, and a set of bool flags I can set.

One thing you may want to work on is figuring out where you will handle errors. Currently, you pass some up, ignore them, print them, print them and return, or print them and exit from all over your code. I'd recommend having a way to delegate showing error/info messages to something controlled by the main function, and either ignoring errors or passing them up (a single error for errors, a slice of them for warnings) to the main function afterwards.

[shermp]

I think I'll merge this as-is for now, unless anyone has any concerns or objections?

I was a bit on the fence regarding the lpath/contentid function changes, as they do kind of rely on the state of the KU object, but hey, I'll run with the changes.

EDIT: probably better actually test it first...

[pgaskin]

I was a bit on the fence regarding the lpath/contentid function changes, as they do kind of rely on the state of the KU object, but hey, I'll run with the changes.

My logic was that even though it requires the path, it is stateless, and thus should go on its own. This also makes unit testing easier for later.

[shermp]

I was a bit on the fence regarding the lpath/contentid function changes, as they do kind of rely on the state of the KU object, but hey, I'll run with the changes.

My logic was that even though it requires the path, it is stateless, and thus should go on its own. This also makes unit testing easier for later.

Fair enough.

Unit testing. Another thing I should be doing, but haven't. You may have noticed, most of my development style is more akin to what authors would call being a "pantser"...

[pgaskin]

The results of the benchmark: https://github.com/shermp/Kobo-UNCaGED/issues/3#issuecomment-493825841

[shermp]

Right, this appears to be behaving itself, and haven't seen any obvious issues, so I'm going to merge this PR. That should make it easier to test all the new changes as a whole.

Thanks again!

[shermp]

Hmm, may have been a little hasty to merge this. Thumbnail generation/saving appears to not be working. I didn't notice until I transferred a larger number of books, and saw Nickel generating thumbnails.

Never mind. Will figure it out tomorrow. It's hardly a critical problem.