Fix issue with Nearby item descriptions

[misaochan]

Seems to fix #823 for me. Could anyone (especially those with a non-English phone language) help test and see if it works for them?

Edit: While we're at it, I renamed Description to Label to prevent any further misunderstandings, lol.

[whym]

Tested and it works for me on Japanese locale.

I'm not sure 'label' is an improvement, although I don't think it's worse either. See #742.

As far as I understand, the correspondence before this patch

• App's 'name' - Wikidata's 'label'
• App's 'description' - Wikidata's 'instance of' value
• n/a - Wikidata's 'description'

After this patch

• App's 'name' - Wikidata's 'label'
• App's 'label' - Wikidata's 'instance of' value

Any alternative idea?

To be clear, as I said in #742 this naming issue is a minor one, and my comment above shouldn't be a stopper anyway.

[misaochan]

Thanks for testing, @whym ! :) The main rationale for me renaming the "instance of" value to Label was to prevent people from confusing it with the actual description (which is named LongDescription). I think what caused the "?" issue to begin with was someone glancing at the code and (understandably) assuming that Description was what he was looking for.

I am good with renaming Label to InstanceOf though if that would be preferred. Anything but Description. :)

[whym]

I see what you mean, but it appears like the app's "description" actually stores the original "instance of" value. (My previous explanation was wrong, too, sorry for that.) To be more precise, the current state is

• Place.name - Wikidata's 'label'
• Place.Description - compact form for Wikidata's 'instance of' value
• Place.longDescription - Wikidata's 'instance of' value verbatim
• n/a - Wikidata's 'description' (which is even longer, by the way, when exists)

So 'Description' and 'longDescription' are related. If the former is to be changed, I think the other should be, too.

I'd be fine with Place.InstanceOf and Place.longInstanceOf.

[misaochan]

I think the main problem is that the current Place.Description is not meant to be displayed to the user at all, but rather just used to determine which icon to use for the Nearby List. Only Place.longDescription is suitable to display to the user. How do you think we should convey this distinction?

How do we access Wikidata's true "description"? That may be more suitable for display to users, as a potential enhancement.

[whym]

How does 'Classification' sound? 'longDescription' could be simply 'instanceOf' then.

How do we access Wikidata's true "description"? That may be more suitable for display to users, as a potential enhancement.

I think it's fairy easy to add a column for it into the query (nearby_query.rq). Not sure how frequently the values are filled given our scope (entities without images) but it wouldn't hurt to add it.

[nicolas-raoul]

Yes getting the Wikidata description in particular language is easy. Most items don't have a description. A lot of those who have are like "Church in Paris", which is not much better than "church" because our users already know where they are. Some description could really provide value though. How about concatenating, or using description wgen available?

…

On Jan 3, 2018 09:05, "Yusuke Matsubara" ***@***.***> wrote: How does 'Classification' sound? 'longDescription' could be simply 'instanceOf' then. How do we access Wikidata's true "description"? That may be more suitable for display to users, as a potential enhancement. I think it's fairy easy to add a column for it into the query (nearby_query.rq). Not sure how frequently the values are filled given our scope (entities without images) but it wouldn't hurt to add it. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1034 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGFBo7bnwvGmKRFpr0mgFhkgF_uahStks5tGuCAgaJpZM4RPoiT> .

[misaochan]

Thanks @whym and @nicolas-raoul !

How does it sound to you guys if I create an issue for further discussing the appropriate names and concatenating the Wikidata description, and copy your comments over to that, so we can merge this if there is nothing else blocking it? Ideally we would like to include this in our next hotfix (which will be released when we fix the Dagger crashes).

[whym]

Sounds like a good plan. Thanks!

[whym]

Two test failures exist:

org.junit.ComparisonFailure: expected:<[airport]> but was:<[desc]>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at fr.free.nrw.commons.nearby.NearbyAdapterFactoryTest.rendererCorrectlyBound(NearbyAdapterFactoryTest.java:76)

org.junit.ComparisonFailure: expected:<[no description found]> but was:<[desc]>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at fr.free.nrw.commons.nearby.NearbyAdapterFactoryTest.rendererCorrectlyBoundForUnknownPlace(NearbyAdapterFactoryTest.java:97)

I haven't checked it closely yet, but the second failure might be related to your change to PlaceRenderer.render().

Locally I used: gradle :app:testBetaDebugUnitTest --tests fr.free.nrw.commons.nearby.NearbyAdapterFactoryTest

[misaochan]

@whym Interesting. I looked at NearbyAdapterFactoryTest.java and it seems that we are hardcoding the longDescription as "desc" in that test suite:

    private static final Place PLACE = new Place("name", Place.Description.AIRPORT,
            "desc", null, new LatLng(38.6270, -90.1994, 0), null);

And then in rendererCorrectlyBound():

        RVRendererAdapter<Place> result = testObject.create(Collections.singletonList(PLACE));
        RendererViewHolder viewHolder = renderComponent(result);

So, it seems that this "desc" value would be passed to all the tests being run on the PLACE object and that is why the comparison is failing? That makes it seem like the issue lies with the test itself. Or am I reading the tests incorrectly?

Same thing happens with UnknownPlace.

[neslihanturan]

It works for me @misaochan , thanks!
Android Version: 4.4.2
But I will wait the answer from @whym before merging.

[whym]

It looks like the test was written by @psh - any chance you can look into this?

[misaochan]

Will likely be doing a release with @maskaravivek 's Dagger fix tomorrow. With this PR, we have two options:

1. Wait to resolve the tests and not roll this fix out to users with tomorrow's release
2. Merge PR, resolve the tests in a separate issue, and roll this fix out with tomorrow's release since the PR fixes the problem for all 3 of us.

[whym]

I think this fixes the tests: whym@698fad6 (well, if I interpret the intention of the tests correctly)

[misaochan]

@whym That looks good to me, thanks. Shall we submit that as a separate PR?

[whym]

Isn't it easier to 'git pull https://github.com/whym/apps-android-commons.git misaochan-fix-description-bug' and update this PR? If doesn't work, I can create a new PR.

[misaochan]

Done, thanks for the tip @whym ! :)

[whym]

/home/travis/build/commons-app/apps-android-commons/app/src/main/res/values-sr-el: Error: Invalid resource directory name

This was reported in #1059, but I'm not sure why your patch is affected. I don't seem to find the values-sr-el directory here?

Locally I cannot reproduce it - maybe we are good to merge despite Travis's error?

[divadsn]

@whym you can merge it, it's a wrong imported translation from translatewiki :)

[whym]

@divadsn yeah, but the odd thing is the offending commit 29e667e is not included in the commits here, or is it?

[misaochan]

I've encountered issues before with Travis failing for all PRs that were rebased on master when the master build is failing. I guess the assumption is that they are based on master therefore unless they fix master's failed build, they all fail by association?

I would be in support of merging. ;)

[whym]

It passed for my branch https://travis-ci.org/whym/apps-android-commons/builds/327725662 with the same changes. Since my master was outdated, Travis automatically merging master seems to explain this behavior.

[VojtechDostal]

Amazing! Descriptions in Czech now work :-)

[misaochan]

Wonderful! Thanks for the feedback @VojtechDostal . :)