commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
1.01k stars 1.19k forks source link

[Bug]: Category not found #4901

Open Ainali opened 2 years ago

Ainali commented 2 years ago

Summary

I serached for a category that exists, but it doesn't show up.

Steps to reproduce

  1. When uploading an image, search for category: Amavenita

Expected behaviour

Category:Amavenita (ship, 2014) should be suggested.

Actual behaviour

"No Categories found"

Device name

Samsung Galaxy S10

Android version

Android 12

Commons app version

3.1.1~1c9267ca0

Device logs

No response

Screen-shots

Screenshot_20220318-180530_Commons

Would you like to work on the issue?

No response

devarsh-mavani-19 commented 2 years ago

@Ainali isn't this the expected behaviour? Because, Amavenita (ship, 2014) is a non-hidden category and I think we are only supposed to show the hidden category in search bar. But I could be wrong.

Ainali commented 2 years ago

Oh, I didn't realize. Then this is a duplicate with #4192 (and a use case for why it is needed, this is how all ships are categorized and the way for a human to identify them).

nicolas-raoul commented 2 years ago

"we are only supposed to show the hidden category in search bar"

@devarsh-mavani-19 I would consider that a bug. If a category is not hidden, then the upload wizard's category search step is supposed to show it.

nicolas-raoul commented 2 years ago

By the way, when I search for "rabbits", the first result is https://commons.wikimedia.org/wiki/Category:Rabbits which is not a hidden category:

adb

devarsh-mavani-19 commented 2 years ago

By the way, when I search for "rabbits", the first result is https://commons.wikimedia.org/wiki/Category:Rabbits which is not a hidden category:

adb

Yes that is how it should work if it is non-hidden, then it should be displayed. However, in case of Ainali it was not displayed because category name contained a year which was older than previous year (Amavenita (ship, 2014)). so according to this issue #750 it should be filtered out.

so just to summarize currently 2 conditions must met 1. it should be non-hidden and 2nd either should not have any year in name or the year should be current or previous year.

Ainali commented 2 years ago

However, in case of Ainali it was not displayed because category name contained a year which was older than previous year (Amavenita (ship, 2014)). so according to this issue #750 it should be filtered out.

Then I would still consider that issue to have been too poorly defined, because the app shouldn't filter out things where the year is part of the identifier of that thing, in this case, the ship that got the name in 2014. (Also, the app should probably not filter out by only older than current year, since it's common to upload files a day or two after they are taken which means that if I upload an image on January 1, I expect to see the category for the year before. Safer would be to filter categories where the year is less than current year minus 2.)

sivaraam commented 2 years ago

I'm presuming the issue #750 was understood properly but the fix for it was a bit loose than it should've been. The relevant code snippet:

return item.matches(".*(19|20)\\d{2}.*".toRegex())
                && !item.contains(yearInString)
                && !item.contains(prevYearInString)
                || item.matches("(.*)needing(.*)".toRegex())
                || item.matches("(.*)taken on(.*)".toRegex())
                || item.matches(".*0s.*".toRegex())
                && !item.matches(".*(200|201)0s.*".toRegex())

That essentially returns true (which means the category is filtered out) when the name:

  1. Contains "19xx" OR "20xx" AND
  2. Does not contain current year AND
  3. Does not contain previous year OR
  4. Contains "needing" OR
  5. Contains "take on" OR
  6. Contains 0s AND
  7. Does not contain "2000s" OR "2010s"

If we were to convert the implicit precedence to explicit ones the condition looks like this:

(((1) AND (2) AND (3)) OR (4) OR (5) OR ((6) AND (7)))

Given short-circuiting evaluation, the condition should've returned true for "Amavenita (ship, 2014)" thus the category getting filtered out.

We could fix this by fixing the loose condition. I'll open a PR.

nicolas-raoul commented 2 years ago

Thanks a lot @devarsh-mavani-19 for finding this out! I had forgotten about this feature. Do we all agree on this?

devarsh-mavani-19 commented 2 years ago

Do we all agree on this?

agree

Discostu36 commented 1 year ago

What is the status of this?

nicolas-raoul commented 1 year ago

@Discostu36 This bug still happens, I just tried on the latest Git code. Do you want to work on it? :-)

Discostu36 commented 1 year ago

Sadly, I lack the skills. I have started learning Kotlin development, but I'm only at the very first baby steps (and don't have much time).

sivaraam commented 1 year ago

For those interested in picking this up, MR #4902 already has the changes to fix this. Only the tests need fixing IIRC. So, it could be used as a base.

I intend to get around to completing it. Do feel free to beat me to it, though 😉