search

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

Various fixes that were broken in github version #11

Closed stoduk closed 8 years ago

stoduk commented 8 years ago

Various things were broken in github version (and 2.2, maybe?) - most fundamentally the breakage that book parsing would not find occurrences correctly (it was finding a very large number for one entity, and none for the others).

I have not tested this on Windows, only OS X. If you can test it, great, if not I'll test it under Virtual Box at some point. Certainly wasn't testing before we release - would be enough to: 1) check that finding Kindle and copying files across still works (ie. both doesn't error and actually does something). 2) check that things "fail" correctly if no Kindle is found

stoduk commented 8 years ago

@szarroug3 just wanted to check you saw the note about testing (or lack of) on Windows. Are you able to try this out?

szarroug3 commented 8 years ago

@stoduk Sorry, got distracted and forgot to mention that I did test on Windows 10. Still working properly. Good job! :)

Tested the following:

Will test other fixes when I get a chance but considering the fact that those other fixes are not OS specific, your testing should be enough.

szarroug3 commented 8 years ago

@stoduk I tested the aliases issue with A Christmas Carol before and after your fix. I got the same results both ways -- it never gave me a lot of mentions for 1 character and none for the rest. Maybe I was missing something? Anyways, I like your way better. It's more complete.

stoduk commented 8 years ago

re. testing: excellent, glad it worked! I think it should avoid a performance issue too (if I understand os.walk, then the code would have bailed early if it found it was looking at a Kindle, but if it didn't find the Kindle magic files it would carry on walking the entire external drive).

re. breakage. Odd that you don't see it! To check - did you try with and without the fix for issue #5? I saw the breakage with any book (not just Xmas Carol I saw it with originally), and on both Windows/OS X.

I think the problem is this behaviour (from aliases.setter):

"".replace(", ", ",").split(",") ['']

That is - if Qt calls back with an empty string for an alias, it was being converted to an array containing an empty string, rather than an empty array as we'd want. This empty string was then being used as an alias - and so gave "\b \b" as a regex pattern, which matches every space character between words (hence the very large number of occurrences), and these were being attributed to whichever alias was the last to be handled (as it was overwriting which term the alias was for in BookParser.init).

[this behaviour of split() is odd, perhaps with some good underlying reason, in that string.split() without any specifier mentioned will remove empty strings from the result (mentioned in the help text explicitly!), but with a specifying it leaves an empty string in there (it doesn't say why it doesn't remove the empty string in this case!). ]

Cheers, Anthony

On 16 May 2016 at 21:56, szarroug3 notifications@github.com wrote:

@stoduk https://github.com/stoduk I tested the aliases issue with A Christmas Carol before and after your fix. I got the same results both ways -- it never gave me a lot of mentions for 1 character and none for the rest. Maybe I was missing something? Anyways, I like your way better. It's more complete.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219545415

szarroug3 commented 8 years ago

I tried it with the 2.1.0 zip.. Maybe I introduced the issue after that release.. Will try again at home

On Mon, May 16, 2016, 4:40 PM Anthony Toole notifications@github.com wrote:

re. testing: excellent, glad it worked! I think it should avoid a performance issue too (if I understand os.walk, then the code would have bailed early if it found it was looking at a Kindle, but if it didn't find the Kindle magic files it would carry on walking the entire external drive).

re. breakage. Odd that you don't see it! To check - did you try with and without the fix for issue #5? I saw the breakage with any book (not just Xmas Carol I saw it with originally), and on both Windows/OS X.

I think the problem is this behaviour (from aliases.setter):

"".replace(", ", ",").split(",") ['']

That is - if Qt calls back with an empty string for an alias, it was being converted to an array containing an empty string, rather than an empty array as we'd want. This empty string was then being used as an alias - and so gave "\b \b" as a regex pattern, which matches every space character between words (hence the very large number of occurrences), and these were being attributed to whichever alias was the last to be handled (as it was overwriting which term the alias was for in BookParser.init).

[this behaviour of split() is odd, perhaps with some good underlying reason, in that string.split() without any specifier mentioned will remove empty strings from the result (mentioned in the help text explicitly!), but with a specifying it leaves an empty string in there (it doesn't say why it doesn't remove the empty string in this case!). ]

Cheers, Anthony

On 16 May 2016 at 21:56, szarroug3 notifications@github.com wrote:

@stoduk https://github.com/stoduk I tested the aliases issue with A Christmas Carol before and after your fix. I got the same results both ways -- it never gave me a lot of mentions for 1 character and none for the rest. Maybe I was missing something? Anyways, I like your way better. It's more complete.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub < https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219545415

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219556833

szarroug3 commented 8 years ago

Ah! I was able to reproduce it!

stoduk commented 8 years ago

Great, glad you confirmed, always better to make sure a problem is correctly understood - especially with unfamiliar code.

On 16 May 2016 at 23:49, szarroug3 notifications@github.com wrote:

Ah! I was able to reproduce it!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219571665

szarroug3 commented 8 years ago

Yeah it only happened if you opened the book preferences before creating the x-ray. That's why I couldn't reproduce it at first.

On Tue, May 17, 2016, 4:37 AM Anthony Toole notifications@github.com wrote:

Great, glad you confirmed, always better to make sure a problem is correctly understood - especially with unfamiliar code.

On 16 May 2016 at 23:49, szarroug3 notifications@github.com wrote:

Ah! I was able to reproduce it!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub < https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219571665

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219667902

stoduk commented 8 years ago

I'm glad we kept talking about this, as I've found another instance of the bug in a different path. I've just submitted a new pull request, hopefully obvious from the issue #13 details.

On 17 May 2016 at 13:51, szarroug3 notifications@github.com wrote:

Yeah it only happened if you opened the book preferences before creating the x-ray. That's why I couldn't reproduce it at first.

On Tue, May 17, 2016, 4:37 AM Anthony Toole notifications@github.com wrote:

Great, glad you confirmed, always better to make sure a problem is correctly understood - especially with unfamiliar code.

On 16 May 2016 at 23:49, szarroug3 notifications@github.com wrote:

Ah! I was able to reproduce it!

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub <

https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219571665

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub < https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219667902

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/szarroug3/X-Ray_Calibre_Plugin/pull/11#issuecomment-219707672