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

Fix english but non-US site being forced to default US site #23

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jmeosbn
Copy link

@jmeosbn jmeosbn commented Dec 22, 2017

Hi there, just some minor fixes noted when selecting the UK audible site - main one is that .com is always used (see first line that's repeated from earlier in def).

@jmeosbn
Copy link
Author

jmeosbn commented Dec 23, 2017

To clarify, passing lang='en' and base='www.audible.co.uk' to SetupUrls, runs into:

if sitetype:
...
if base in sites_langs : (yes, it is)

then, base=intl_sites[lang]['url'] (looks up url for en in intl_sites)

Which overwrites base with www.audible.com - so I think that last line is mistakenly duplicated from further down in def where it appears in the else clause of if sitetype:.

Presumably, same will happen if wanting to use www.audible.com.au

@macr0dev
Copy link
Owner

I'll be back home on Tuesday and can run some thorough tests to see what's going on here. The initial international support based on library language was done by another developer and I wrapped the option to manually select it around that code. I'm working on a laptop in a different U.S. state than usual due to holiday traveling.

@jmeosbn
Copy link
Author

jmeosbn commented Dec 24, 2017

Yes, I think that line must either be a duplication error, or remnant from some history where non-US-but-still-english sites were not added.

Commenting that single line seems to be all that's required, will probably rebase on latest to fix merge conflict on current head.

@jmeosbn jmeosbn force-pushed the pull-requests/co-uk-fixes branch 2 times, most recently from de8c18f to 81c7fcd Compare December 24, 2017 20:07
@jmeosbn
Copy link
Author

jmeosbn commented Dec 24, 2017

After refactoring of the parameter localisation stuff into sites_langs along the other changes above, I get the following results with the same test library of six books, two of which have mangled info:

UK:

  • matches as expected to actual co.uk site, with the two mangled albums not matched (though that author does not seem to be listed on the UK site)

US:

  • matches better than before, actually matching the two mangled albums also (which did not happen until now). 💯

However, initial library refresh still seems to skip updating despite matching 100% - it actually [edit: I've committed changes that seem to have resolved this issue)

@@ -31,78 +31,73 @@ def json_decode(output):
THREAD_MAX = 20

intl_sites={
Copy link
Author

Choose a reason for hiding this comment

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

I think intl_sites could be removed and a search in sites_langs used instead?

@jmeosbn jmeosbn force-pushed the pull-requests/co-uk-fixes branch 3 times, most recently from a0ea30b to 3b3e459 Compare December 29, 2017 17:27
@@ -102,12 +102,15 @@ def SetupUrls(sitetype, base, lang='en'):
# Match titles using more flexible keyword search
if Prefs['keyword_searching']:
urlsearchtitle = "advsearchKeywords="
else:
# fallback for .com title search outside US
urlsearchtitle += "{0}&searchTitle="
Copy link
Author

Choose a reason for hiding this comment

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

A little inelegant but it's a one line fix vs. something more drastic! 😱

@jmeosbn
Copy link
Author

jmeosbn commented Dec 29, 2017

Added fallback workaround for title/searchTitle - will conflict with #28 so pulled that to here:

I've been having better results using advsearchKeywords search instead of explicitly specifying a title search (which is much more strict and fails on things like numbers prefixed etc.). Author can still be specified which results in a more flexible title search still sensibly constrained when author known.

Nice side effect is that advsearchKeywords= survives when searching the .com site from outside the US - unlike title=.

@jmeosbn jmeosbn force-pushed the pull-requests/co-uk-fixes branch 2 times, most recently from 4acd871 to 0faa20d Compare December 31, 2017 20:14
@jmeosbn
Copy link
Author

jmeosbn commented Dec 31, 2017

Looks like the title/searchTitle issue when using .com outside the US got fixed today.. so the "Provide additional search parameter as fallback" commit may no longer be needed.

However, it doesn't hurt to have it and it provides a fallback that could simplify things going forward.

edit: switches back and forth so best keep it in..

@jmeosbn jmeosbn force-pushed the pull-requests/co-uk-fixes branch 4 times, most recently from fad3bb1 to 079457c Compare January 7, 2018 19:58
Copy link
Author

@jmeosbn jmeosbn left a comment

Choose a reason for hiding this comment

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

I've tested this 33f6c53 and the other related 079457c commit on all supported sites and there are now class names and/or the new json style pages are being served (on and off like .com).

Can somebody in the US or elsewhere (I'm in the UK) double check this please? 👍

P.S. 1a3f5bb is cool too yes?

@jmeosbn
Copy link
Author

jmeosbn commented Jan 25, 2018

note: additional commits pruned back as I'm unable to keep stale PRs from conflicting between each other and my current local copy

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

Successfully merging this pull request may close these issues.

2 participants