szarroug3 / X-Ray_Calibre_Plugin

X-Ray Creator plugin for Calibre
http://www.mobileread.com/forums/showthread.php?t=273189
GNU General Public License v3.0
57 stars 12 forks source link

Dev/device drivers #41

Closed szarroug3 closed 8 years ago

szarroug3 commented 8 years ago

Fixes #28:

Should also fix #34

Issue: trying to use the plugin while Calibre is still communicating with the device causes an error saying "Error communicating with the device" -- they're probably trying to talk to it at the same time

stoduk commented 8 years ago

Sorry for slowness, busy over here.. Some general comments here, other comments annotating the diff

Generally - it works, so infinitely better than we had, great! Quite a few of the comments are possible improvements, or quirks in the existing code. I've fixed one tiny bug (not clearing error status) in my branch, and I think stripping the query from the goodreads URLs will improve readability.

1) Searching Urgh, goodreads sucks :) Only being able to search by title or author seems mad, when we have both.

As you pointed out this doesn't work well for some books (eg. A Christmas Carol) - in that case just searching by title seems to do a good job. Could we try moving to that? There seems to be some magic on the goodreads side to guess what book you were after. It might be necessary to then check the author in the books found, perhaps returning the first match.

2) Searching by ASIN or ISBN Goodreads supports searching by either of these IDs, which we can grab from books in many cases - and is less likely to give the problems mentioned in the previous point.

Is there a reason ASIN searching has been discarded?

3) Aliases Your comments in Gitter made it sound like goodreads added aliases, but shelfari already had them.. So not sure a change was needed.

In the case you gave I don't see why we'd want to expand "Tiny Tim" - as matching Tiny throughout a book is not going to end well. Having said that an alias could also be a pseudonym rather than a nickname - in which case expansion makes sense as much as it does for the real name (eg. Marge Simpson and Hootie McBoob are both names that we could validly expand).

So I guess we need to decide if aliases are more likely to be pseudonyms or nicknames - I'd believe nicknames are more likely (so expansion doesn't make sense) but that is a feeling based off little hard facts :)

Could be that a user preference is needed to let the user decide..

4) Book preferences dialog This has always been a bit cramped for me, I was going to raise it one day, I wasn't sure if this was an OS X problem or more general. Labels are often wider than the boxes/buttons allow - and with the new goodreads URL being so huge this is even worse.

Some options:

import urlparse
url = urlparse.urlunparse(urlparse.urlparse(url)._replace(query=None))

So we could either:

So being able to read the URL isn't important, especially if we add some better way to validate it.

5) I'm not a fan of having lower case names in the user-visible text (eg. book config dialog), it looks messy to my eye. Probably nicer to .lower() it after the user sees it (we'll need to .lower() it anyway in case the user has edited it).

Regardless: with the new alias handling we now have a mix of .lower() and unmodified strings which looks messier :) eg. "ebenezer, Scrooge" and "timothy, tiny, tim, Tiny Tim", but not "jacob, marley" (I assume all lower because he has no aliases)

szarroug3 commented 8 years ago

1) Searching Yeah it does really suck. There has to be a better way to do this. I like the idea of searching by title then checking the authors -- in the case of multiple books with the same name by different authors.

2) Searching by ASIN or ISBN The only way to get the ASIN is from Amazon. I saw that many people on MobileRead were complaining about the Amazon page not being found. After investigating, I found that some Amazon pages redirect you sometimes not only to a different page but to a different protocol (HTTPS or HTTP). HTTPConnection and HTTPSConnection don't handle that at all. They basically just tell you that you've been redirected and then you have to handle it yourself -- in the case of redirecting to a different protocol, we'd have to open a new connection altogether. I really want to keep keep-alive connections since I'm sure there are people who use this plugin on a huge list of books at once and having to open a connection everytime is very slow. I haven't been able to find any other good way to do keep-alive connections but we can look into this again.

3) Aliases I said that because we weren't getting aliases directly from Shelfari as we are from Goodreads. This could affect #37 so I thought it was worth mentioning as it could affect any code you've already written. I find that most aliases are just other names they go by (Vin in Mistborn is called Valette Renoux and Vin Venture a lot). I like the idea of letting the user decide this.

4) Book preferences dialog Agreed. It is a bit cramped. I LOVE replacing the url edit box with something more useful. I think the title and author is probably sufficient. Maybe even add the book's description (that could get cramped so maybe just a portion of it?)

5) Lower Case Names Yeah I don't like it either but it didn't bother me too much. I was mostly worried about getting a working version out and fixing little things like this in the next version.

I'll add issues for each other the things we need to fix/change.