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

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

Open
nicolas-raoul opened this issue Mar 30, 2024 · 32 comments
Open

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

nicolas-raoul opened this issue Mar 30, 2024 · 32 comments

Comments

@nicolas-raoul
Copy link
Member

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

@rohit9625
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member Author

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

@ShashwatKedia
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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

@rohit9625
Copy link
Contributor

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
Copy link
Member Author

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

@rohit9625
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member Author

@rohit9625 Fantastic, thanks a lot! :-)

@sivaraam OK for going ahead with Jetpack Compose?

@misaochan
Copy link
Member

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

@rohit9625
Copy link
Contributor

+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
Copy link
Member

+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
Copy link
Member

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

@rohit9625
Copy link
Contributor

rohit9625 commented May 24, 2024

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

Brief discussion about Compose migration: #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
Copy link
Contributor

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

@rohit9625
Copy link
Contributor

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

Current behavior:-

  • Opening a folder and selecting images works fine but when navigate back and again come to the folder from where I selected pictures, there are no pictures selected at all. However, we can still upload those pictures by hitting on upload.
  • This time select some pictures from one folder and navigate to another folder and selecting some more pictures from there will eventually clear off the previously selected images.

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
Copy link
Member Author

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: #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 #5802 for it. :-)

@rohit9625
Copy link
Contributor

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
Copy link
Member Author

If you prefer, feel free, yes. 🙂

@nicolas-raoul
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants