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

Add geolock and language filters in the add-on browser #829

Closed
wants to merge 5 commits into from
Closed

Add geolock and language filters in the add-on browser #829

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 29, 2012

This PR adds

  • geoip support to system info
  • ability to flag add-ons as geolocked
  • ability to filter out the geolocked add-ons in the add-on browser
  • ability to specify add-on languages
  • ability to filter out foreign languages in the add-on browser

i'm not entirely happy about the filtering happening in the directory class, but the alternative is fuglier (namely reconstruction addon-id's from the just-returned list of fileitems inside the browser). it should not hurt though, just a purist thing really.

is this something we want? i think a lot of users will appreciate both of these..

@JezzX
Copy link
Contributor

JezzX commented Mar 29, 2012

Nice work spiff I can hear the out cry now "XBMC is tracking my location" :) even though weather already does it
And yeah the fact that you can disable it is good

@jmarshallnz
Copy link
Contributor

Alternate is setting properties for the bits you want to filter in the FillItem stuff then filtering on the property in the browser?

@garbear
Copy link
Member

garbear commented Mar 30, 2012

This is related to PR #830. It seems location is starting to play a bigger role in XBMC. We should probably consider pulling our scattered location methods (wunderground, freegeoip, wifi, maybe android/iphone gps in the future) into an addon.

spiff added 2 commits April 3, 2012 13:18
this allows listing the countries the content is locked to for
add-ons providing geo-locked content
@ghost
Copy link
Author

ghost commented Apr 3, 2012

rebased to use an add-on to grab region.

@dersphere
Copy link
Contributor

FYI: freegeoip.net has a request limit of 1000/h (which is ~1.1 request every 4 seconds). If this limit is reached property won't be set and never retried.
EDIT: sry. I re-read the source, limit is per IP, forget my comment.

spiff added 3 commits April 3, 2012 15:14
allows specifying the language(s) of the content the add-on provides
…owser.

English add-ons are always included. Setting defaults to false
@ghost
Copy link
Author

ghost commented Apr 3, 2012

and filtering moved to window.

@theuni
Copy link
Contributor

theuni commented Apr 3, 2012

If we're going to do geoip lookups, we need to provide them ourselves rather than relying on some 3rd party site to stay up and fulfill the requests AND not get pissed at us in the process. This has bitten us more than once before.

@theuni
Copy link
Contributor

theuni commented Apr 3, 2012

As it wasn't apparent in my tone above..

We should be able to handle this easily enough. Addon downloads already do geoip lookups in order to forward to the closest mirror, we'd just need to add a quick function to expose that functionality externally.

@jmarshallnz
Copy link
Contributor

If we're going to use our servers, then we can potentially get away with doing it without the add-on - not that it really matters - either is fine in my opinion (the add-on could be used easily by other scripts and we don't need to bother exposing it, though that would be trivial).

Rest of the code looks fine from a quick scan.

@garbear your thoughts on the add-on thing?

@garbear
Copy link
Member

garbear commented Apr 4, 2012

script.library.geoip suffices. Content providers block by IP, so it doesn't make sense for us to use any other locationing mechanism besides geo-ip for geolock.

This duplicates functionality from the wunderground add-on, so I think we should setting on a single URL for ip lookups.

@garbear
Copy link
Member

garbear commented Apr 5, 2012

If this feature does go the way of a location addon, I got the perfect icon for it :)
DefaultAddonLocation.png

Edit: copied the wrong link

@JezzX
Copy link
Contributor

JezzX commented Apr 5, 2012

garbear getting off topic but your image is
"Error (403)
It seems you don't belong here! You should probably sign in. Check out our Help Center and forums for help, or head back to home."

@garbear
Copy link
Member

garbear commented Apr 5, 2012

fixed (maybe)

@JezzX
Copy link
Contributor

JezzX commented Apr 5, 2012

yep fixed and nice but its an addon not an addon category right ?

@garbear
Copy link
Member

garbear commented Apr 5, 2012

ya. it was just a thumb i drafted a year or two ago

@theuni
Copy link
Contributor

theuni commented Apr 23, 2012

got our own setup: http://geoip.xbmc.org/json/

I think that's all that's needed?

@jmarshallnz
Copy link
Contributor

For this, country is probably enough. For weather, we'd probably want nearest city if possible?

@theuni
Copy link
Contributor

theuni commented Apr 23, 2012

Ok, added. But the results are pretty poor.

Compare the free results: https://www.maxmind.com/app/lookup_city?type=geolite
to the commercial ones: http://www.maxmind.com/app/mylocation

@MartijnKaijser
Copy link
Member

both came with same results for me

@jmarshallnz
Copy link
Contributor

Same for me - both are equally inaccurate (it thinks I'm 600km away).

@da-anda
Copy link
Member

da-anda commented Mar 1, 2013

for me the location returned by our server is more accurate but still 70km away

@garbear
Copy link
Member

garbear commented Mar 1, 2013

makes my geo-wifi patch look better and better ;)

@da-anda
Copy link
Member

da-anda commented Sep 2, 2013

I know this one is on hold atm, but what are the plans? Trying to bring focus to our oldest PRs so they don't get forgotton.

tru pushed a commit to plexinc/plex-home-theater-public that referenced this pull request Nov 13, 2013
@MartijnKaijser
Copy link
Member

@tamland maybe you can pick this up if you are interested

@MartijnKaijser MartijnKaijser modified the milestones: Temporary freezer until devs have time, Abandoned, obsolete or superseeded May 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature non-breaking change which adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants