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

Show Tibet as part of China on map #11930

Merged
merged 15 commits into from Sep 12, 2017

Conversation

Projects
None yet
@sgiehl
Member

sgiehl commented Aug 3, 2017

This Pull Request is not yet complete.

There might be still some places where old data might be shown as "Unknown" now.

And as old visits won't be changed, segments for showing visits from Tibet might have incorrect results, need to check if there is an easy way to handle that.

fixes #6006

@sgiehl

This comment has been minimized.

Member

sgiehl commented Aug 3, 2017

@Findus23 We need to remove the flag for Tibet as soon as this gets merged in order to get the tests fixed

@gameblabla

This comment has been minimized.

gameblabla commented Aug 4, 2017

I hope this doesn't get merged.

@phodal

This comment has been minimized.

phodal commented Aug 4, 2017

@gameblabla Hi, had your used a map? such as Google map, Bing map or Apple map, the last commit about "free tibet" was a joke!

@gameblabla

This comment has been minimized.

gameblabla commented Aug 4, 2017

@phodal, i know them but i do not get your point.
Perhaps it could be done in a different way, keep tibet as a country but mark it as a "disputed territory".

@fxzjshm

This comment has been minimized.

fxzjshm commented Aug 4, 2017

@gameblabla Tibet belongs to China, and there is no dispute

@gameblabla

This comment has been minimized.

gameblabla commented Aug 4, 2017

@fxzjshm There's no need to be hostile, i'm trying to find a good compromise. There are people who agree with you and there are people who don't agree. I believe piwik should not be one-sided.

@sgiehl, forgive me but is my idea okay with you ? This might help with the "Unknown" stuff without breaking things. (i admit i know very little php)

@phodal

This comment has been minimized.

phodal commented Aug 4, 2017

@gameblabla, did any country recognized Tibet was a country ? America? England ? France? If so, could you list the country name ?

May be for now, India thought Tibet was a country? Because a lot of Indian army was within Chinese borders. Are you come from India, just prepare discuss the disputed territory side of China?

I remember in 1906, according to Convention Between Great Britain and China Respecting Tibet . The Government of China also undertakes not to permit any other foreign State to interfere with the territory or internal administration of Tibet.

So please don't permit interfere with the territory or internal administration of Tibet.

@gameblabla

This comment has been minimized.

gameblabla commented Aug 4, 2017

I'm not sure if this is the place to discuss about this but i will anyway.
@phobal There's India which has troops in Ladakh, which is close to the tibetan border. Britain also recognized it as an autonomous region (as the "administration of Tibet" in the Convention of 1906) until 2008. Before Nixon, the US also recognized Tibet. To this day, it is still disputed by the Dalai-Lama.
So I think it is enough proof that the territory, while indeed under China's control, is still in dispute.
Hence my idea

@fengkaijia

This comment has been minimized.

Contributor

fengkaijia commented Aug 4, 2017

@gameblabla There's no point to discuss historical claims on this. The Taiwan Authority of China claims Outer-Mongolia until 2002, then should we prepare an addition map that has Mongolia inside China? If we don't build map base on the current situation, then I'm sure we need to build hundreds of maps. Like how about a map for the empire that sun never sets? Or a map for the Mongol Empire? No, that's not cool.

@gameblabla

This comment has been minimized.

gameblabla commented Aug 4, 2017

@fengkaijia, the difference is that no one claims the territory for the "Mongol Empire" or "the empire that sun never sets" whereas it's different with Tibet, even now. That's not counting Australia welcoming tibetan refugees.

Ultimately, it's up to piwik's devs to decide if they should go ahead with the merge or not.

@fengkaijia

This comment has been minimized.

Contributor

fengkaijia commented Aug 4, 2017

@gameblabla No no you are mixing up with the refugee concept. The Russian welcomed Edward Snowden, it doesn't mean Russia claimed the US territory. Tibet is a part of China, that's what Australia claims:

The Australian side confirmed the position of successive Australian Governments since 1972 that Australia respects China's sovereignty and territorial integrity, including in relation to Tibet and Xinjiang.

@gameblabla

This comment has been minimized.

gameblabla commented Aug 4, 2017

@fengkaijia, yes you're right, i wasn't denying that. But still, i was pointing out that some people (including tibetans in australia) still do not agree with China on this.

I'll let the devs decide on this.

@sgiehl

This comment has been minimized.

Member

sgiehl commented Aug 4, 2017

There is only one reason why we will change this. We want to show a map that matches other maps of analytics tools like Google Analytics and others. So there won't be any reason in the future to discuss stuff like that. Sure, there might still be some stuff incorrect, but this one can be changed by reverting an old commit, while other changes aren't easily possible without #11929

@gameblabla

This comment has been minimized.

gameblabla commented Aug 4, 2017

I understand the technical reasons.
Still, it is not desirable and having this PR being merged would be disappointing.
The whole point of Piwik was to not become like Google Analytics, if it did,
then what is the point ?

If this PR gets merged, then most likely you'll get more issues about Taiwan and (eventually) Hong Kong.
Do you want to get even more pressure ? Sometimes, it is best not to cooperate to avoid further harassment.

I do wonder what @mattab think of this? He was the one who made the original commit after all.

@sgiehl

This comment has been minimized.

Member

sgiehl commented Aug 4, 2017

@gameblabla I didn't mean Piwik wants to become like Google Analytics.
And I don't want to continue any discussion about that. We aim to use official maps and this is a first step. So if official maps will change we aim to change our maps as well.
And if matomo-org/matomo-map-generator#1 will help to fix our map generator and we generate some new maps, Tibet would be part of China again as well.

@gameblabla

This comment has been minimized.

gameblabla commented Aug 5, 2017

Fine then, i will not insist further. I am considering a fork.
Have a nice day

@sgiehl

This comment has been minimized.

Member

sgiehl commented Aug 5, 2017

Remaining failing tests should be fixed as soon as flag was removed from piwik/icons

@fengkaijia

This comment has been minimized.

Contributor

fengkaijia commented Aug 6, 2017

@sgiehl I just sent a PR matomo-org/matomo-icons#14 to remove the flag.

@fengkaijia

This comment has been minimized.

Contributor

fengkaijia commented Aug 6, 2017

Plus, I wonder if the UserCounty is applied for old visit, plugins/UserCountry/Visitor.php shall also be changed.

And the Archive reader, oh that sounds like a very complicated job.

@Findus23

This comment has been minimized.

Member

Findus23 commented Aug 6, 2017

@fengkaijia's pull request is merged and reflected in #11904

@WG-Com

This comment has been minimized.

WG-Com commented Aug 6, 2017

@gameblabla DO NOT Psycholagny OK?If you want Tibet to be independent,JUST FIGHT WITH PLA, I love to see you get shit out by them , you'r country, india ? usa? france? england? all defeated by PLA

@sgiehl sgiehl force-pushed the mapupdate branch from 6be51bf to aa4b0cb Sep 11, 2017

@@ -75,6 +82,14 @@ public function getRegion($idSite, $period, $date, $segment = false)
$separator = Archiver::LOCATION_SEPARATOR;
$unk = Visit::UNKNOWN_CODE;
// show visits tracked as Tibet as region of China
$dataTable->filter('GroupBy', array('label', function($label) {

This comment has been minimized.

@tsteur

tsteur Sep 11, 2017

Member

As those GroupBy filters are quite slow I wonder if we could maybe remove this again in Piwik 4 and somehow mention this in an issue or so? Or maybe we could have first a simple check whether there is actually any row containing this label? Like if($dataTable->getRowFromLabel('1|ti')) { ... groupby filter...}

Same could be done in countries. This would not check lowercase but don't know if we actually have it in different cases in the DB?

This comment has been minimized.

@sgiehl

sgiehl Sep 11, 2017

Member

Checking if an entry exists before adding the group by filters should work, will test that.
Removing in Piwik 4 might not be that easy. We could add an update script to update all entries in log_visit table (which I would do in next major release only as it might run a bit longer), so we can remove some of the checks for visitor log & profile, but the group by would still be required if the archived reports aren't reprocessed.

This comment has been minimized.

@tsteur

tsteur Sep 11, 2017

Member

It is fine to leave it in there as long as we do the check if table has the row at all.

@@ -22,6 +22,10 @@
*/
function getFlagFromCode($code)
{
if (strtolower($code) == 'ti') {

This comment has been minimized.

@tsteur

tsteur Sep 11, 2017

Member

Just wondering if this is still needed after moving ti to cn ? Or is it just a fallback?

This comment has been minimized.

@tsteur

tsteur Sep 11, 2017

Member

I see... it might be for visitor log or so

@@ -1355,10 +1355,10 @@
$.extend(UserCountryMap, {

This comment has been minimized.

@tsteur

tsteur Sep 11, 2017

Member

Map seems to still work 👍

@tsteur

I'd only try to improve the GroupBy to do it only when needed and when the table actually contains such a label, will improve performance a bit as groupBy can take a bit of time (in this case it wouldn't take too long as it would not actually group many, but be still quite a bit faster and take less memory etc). Otherwise good to merge.

@tsteur

tsteur approved these changes Sep 11, 2017

@sgiehl

This comment has been minimized.

Member

sgiehl commented Sep 12, 2017

Last changes weren't compatible with Datatable Maps. Just pushed an update. Let's see if all tests pass now and then we can finally merge

@tsteur

This comment has been minimized.

Member

tsteur commented Sep 12, 2017

👍

@sgiehl sgiehl merged commit 3cb7169 into 3.x-dev Sep 12, 2017

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sgiehl sgiehl deleted the mapupdate branch Sep 12, 2017

'label',
function ($label) {
if ($label == '1|ti') {
return 'cn';

This comment has been minimized.

@fengkaijia

fengkaijia Sep 13, 2017

Contributor

@sgiehl Thanks for the merge. Should the return value be 14|cn?

This comment has been minimized.

@sgiehl

sgiehl Sep 13, 2017

Member

sure. I'll fix that typo

@mattab mattab modified the milestones: 3.2.0, 3.1.1 Sep 14, 2017

@Glisse1

This comment has been minimized.

Glisse1 commented Sep 21, 2017

Chinese pressure is so big that no one can resist it seems.

Say they conquer korea and japan today like they did the massacre in Tibet, next year piwik will update the new map with the new "chinese territories" :)

@matomo-org matomo-org locked and limited conversation to collaborators Sep 21, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.