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.2k forks source link

Nearby -> Custom picker: Only allow 1 picture to be selected #5674

Open nicolas-raoul opened 6 months ago

nicolas-raoul commented 6 months ago

Currently several pictures can be selected, which is not OK from a Wikidata point of view.

rohit9625 commented 6 months ago

Okay, I got it :) But one question. By Custom Picker, did you mean this, Mr. Nicolas? WhatsApp Image 2024-03-30 at 19 14 51_dcc5ec51

If yes, then I would like to work on this task.

ShashwatKedia commented 6 months ago

@rohit9625 I think by custom picker Nicolas means the custom gallery in the app which can be accessed from the Nearby section, which looks something like this:

Custom picker

There are 2 ways to access this: first from the contributions screen (+) and a second from the nearby when you click on a pin near you then you see a (+)

rohit9625 commented 6 months ago

Yes, I tried opening this from Nearby fragment but I got only these two options: WhatsApp Image 2024-03-30 at 19 37 18_a5b8b692

nicolas-raoul commented 6 months ago

It is a recent improvement, only available on lastest main branch.

ShashwatKedia commented 6 months ago

@nicolas-raoul, just a small doubt: why should we not allow users to upload multiple images? Let's say the user wants to upload multiple angles of the nearby place.

nicolas-raoul commented 6 months ago

Each Wikidata item should have at most one P18 property. The picture that best represents it. The other images that represent the same item can be linked to it as "depictions".

rohit9625 commented 6 months ago

So, should we also need to allow for 1 picture to be selected from the selector that is on the screenshot I uploaded?

nicolas-raoul commented 6 months ago

@rohit9625 Yes totally! If no issue exists about this yet, you may want to create one indeed. :-)

rohit9625 commented 6 months ago

Okay 👍, I will start working on the current one first. Also, now I am getting the custom-selector option at the Nearby, Thanks :)

nicolas-raoul commented 4 months ago

@rohit9625 Are you still working on this? If not feel free to unassign :-)

rohit9625 commented 4 months ago

@rohit9625 Are you still working on this? If not feel free to unassign :-)

I was busy with my academics in the past days but now I have time to consider this and other assigned issues. I'm starting to work on it today :)

rohit9625 commented 4 months ago

Hey @nicolas-raoul , I was planning to recreate the whole custom selector using Jetpack Compose and optimize its performance as I mentioned in the GSoC proposal. So, if the selected intern @kanahia1 is not planning to work on it, I want to work on it.

I will resolve this one and other related issues during this period. It'll not take much time :)

Is there any issue opened already related to this or should I create one?

nicolas-raoul commented 4 months ago

@rohit9625 Fantastic, thanks a lot! :-)

@sivaraam OK for going ahead with Jetpack Compose?

misaochan commented 4 months ago

+1 for Jetpack Compose, if we aren't using it already. It seems to be the official recommendation for future-proof UI development.

rohit9625 commented 4 months ago

+1 for Jetpack Compose, if we aren't using it already. It seems to be the official recommendation for future-proof UI development.

Am I good to go then?

misaochan commented 4 months ago

+1 for Jetpack Compose, if we aren't using it already. It seems to be the official recommendation for future-proof UI development.

Am I good to go then?

Sorry, I think we should probably wait for @sivaraam , I was just voting. :)

sivaraam commented 4 months ago

I'm fine with using it if it interacts well with our existing views. :-)

rohit9625 commented 4 months ago

I'm fine with using it if it interacts well with our existing views. :-)

I'll make it interact with the views by following the migration strategy from Android Developers Documentation. Thank you for your approval :)

rohit9625 commented 4 months ago

Hey @nicolas-raoul and @sivaraam I had to upgrade the compileSDK version to 34 and gradle version to latest one, in order to use the latest jetpack compose plugins.

Now, the compose UI can be easily integrate with the views :)

Is it okay?

sivaraam commented 4 months ago

If it doesn't break / interact badly with other existing features, it's a fine thing. But I'm a bit doubtful about bumping compileSdk but leaving targetSdk as-is 🤔

rohit9625 commented 4 months ago

I didn't try to upgrade target SDK because when I got this error, it said that it could be done separately. Screenshot from 2024-05-25 21-24-42

However, I don't know how this target SDK will affect the whole project. So I haven't touched that yet. Should I upgrade this now?

sivaraam commented 4 months ago

However, I don't know how this target SDK will affect the whole project. So I haven't touched that yet. Should I upgrade this now?

If it seems safe to bump compileSdk alone, it should be fine to just do that. Bumping targetSdk would likely introduce a new set of changes that are better explore outside the scope of your change 🙂

rohit9625 commented 3 months ago

Hey @nicolas-raoul, I was working on implementing the screens in Jetpack compose but couldn't find the code responsible for showing Already actioned images.

Can you please guide me to the code?

nicolas-raoul commented 1 month ago

@rohit9625 Sorry I have not been able to help unfortunately! I unassign for now, but if you are you still working on this, please let us know. If no answer, someone else may be assigned to it. Thanks a lot. :-)

rohit9625 commented 1 month ago

Hey @nicolas-raoul, Sorry, I was unable to resolve this issue yet. The Last time when I was working on this, several dependency and gradle problems arrived and due to my academics and building a project for my portfolio, that time I couldn't give the time it required. However, the issue(Only allowing 1 picture to be selected) is not a big one and yeah, I can resolve it. But, I wanted to migrate to compose.

That's why I have some questions regarding it:-

  1. Does this project have any specified architecture or I can follow the recommended architecture from Android, i.e., (Data, Domain, and Presentation Layer)?
  2. Migrating will entirely change the custom-selector package. Is it fine?
  3. Do we have some kind of UI tests for custom-selector?
nicolas-raoul commented 1 month ago

Brief discussion about Compose migration: https://github.com/commons-app/apps-android-commons/issues/5582

  1. Recommended Android architecture is fine, I believe.
  2. That's OK I guess.
  3. We have unit tests for the custom selector, not sure about UI-specific tests though.
rohit9625 commented 1 month ago

I will start working on this once the targetSDK is upgraded to 34.

rohit9625 commented 1 month ago

Hey @nicolas-raoul, I want to discuss something about the custom picker.

Current behavior:-

Do we have a feature like if the user selects some pictures from a folder, then navigates back to the folders list, then goes to another folder, selects some more pictures, and performs actions on all selected pictures as a whole?

Because, if we have this feature then it seems buggy. If don't have this feature then I can implement this in the jetpack compose variant only if it is required.

One more thing is that in the custom picker user clicks to select the picture and long presses to open it. However, I think this is kinda opposite behavior. What do you think?

nicolas-raoul commented 1 month ago

if the user selects some pictures from a folder, then navigates back to the folders list, then goes to another folder, selects some more pictures, and performs actions on all selected pictures as a whole?

That would be a really great feature to have! Actually just two days ago I was thinking that it would be nice if the app could do that (I had a set of 10 pictures to upload but only one required a privacy blurring and thus was saved in the blurring app's folder).

The app's current behavior when coming back is kind of unspecified. I created an issue for this: https://github.com/commons-app/apps-android-commons/issues/5801

One more thing is that in the custom picker user clicks to select the picture and long presses to open it. However, I think this is kinda opposite behavior. What do you think?

Great idea! I created https://github.com/commons-app/apps-android-commons/issues/5802 for it. :-)

rohit9625 commented 1 month ago

Thank you for specifying and creating new issues. However, I want to address those issues as well as this issue in a single PR. May I?

nicolas-raoul commented 1 month ago

If you prefer, feel free, yes. 🙂

nicolas-raoul commented 1 month ago

Can you comment on them so that I can assign you?