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

[hacktoberfest] - image pick option #226

Closed
Canato opened this issue Sep 30, 2021 · 22 comments · Fixed by #233
Closed

[hacktoberfest] - image pick option #226

Canato opened this issue Sep 30, 2021 · 22 comments · Fixed by #233
Assignees

Comments

@Canato
Copy link
Member

Canato commented Sep 30, 2021

Description

Today the user can use the library to pick images from camera, gallery or documents

But the user cannot select that is from camera only, for example. So we need to add a parameter to the contract that define which intents we will use for selection.

Possible solution:

PickImageContract would receive a data class, like we have in CropImageContract and this data class will contain booleans for each type of selection camera/gallery/etc in the case that if you only choose one it open directly

@jashasweejena
Copy link

Hey, I would like to work on this

@Canato
Copy link
Member Author

Canato commented Oct 4, 2021

Thanks @jashasweejena feel free to start.

@jashasweejena
Copy link

Thank you @Canato :) !

@Zikstar
Copy link
Contributor

Zikstar commented Oct 5, 2021

Hi @Canato, I don't know how large the scope of this work will be. But I'll like to help out with this, or writing tests(unit/integration)

@Canato
Copy link
Member Author

Canato commented Oct 5, 2021

Hey @Zikstar any help will be amazing! Try and let me know

@Zikstar
Copy link
Contributor

Zikstar commented Oct 5, 2021

I've started working on it @Canato . Do you also want users to be able to specify where they want to pick from as part of CropImageContractOptions? That way in the CropImageActivity, its not a hardcoded value
Something like what I have in the screenshot below

Screen Shot 2021-10-05 at 14 05 01

.

@Canato
Copy link
Member Author

Canato commented Oct 5, 2021

Yes @Zikstar nice approach, we probably need to update options to cropImageOptions and I expected that our PIckImageOptions will have some default value that always show camera and gallery, giving the user the option to update.

@Zikstar
Copy link
Contributor

Zikstar commented Oct 5, 2021

Cool I'll update that @Canato

@jashasweejena
Copy link

jashasweejena commented Oct 5, 2021

Hey @Canato @Zikstar, I was actually working on the same issue. Do you think we should reassign some tasks?

@Canato
Copy link
Member Author

Canato commented Oct 5, 2021

@jashasweejena can keep working, already saw some PRs that are open and after people find bugs they do not fix, because some stuffs in this library are hard.

If one of you merge before the other, for sure we will have improvements from the second person to put on top of it!

I believe we can bring a good solution together and everyone will be able to help and get some hactoberfest points ^^

@jashasweejena
Copy link

Cool @Canato ^_^

@jashasweejena
Copy link

Hey @Canato, as it appears that @Zikstar has made some good progress on the issue I don't think it makes sense for me to work on this issue. Is there any issue you'd want me to look into?

@Canato
Copy link
Member Author

Canato commented Oct 5, 2021

Sorry for the messup @jashasweejena feel free to check what Zikstar already did and think about improvements or think in any of the other issues we have open

Tests and documentation and never enough ^^

@Zikstar
Copy link
Contributor

Zikstar commented Oct 5, 2021

Sorry to barge in @jashasweejena. This is actually my first open-source contribution, and it's a library I'm familiar with and use at work. So I really wanted to contribute something

@jashasweejena
Copy link

Sure @Canato. No problems @Zikstar, I'm glad that you completed your first contribution. I'm a rookie myself and I know it feels awesome ☺️

@Canato Canato linked a pull request Oct 5, 2021 that will close this issue
4 tasks
@Zikstar
Copy link
Contributor

Zikstar commented Oct 5, 2021

@Canato How do I update wiki? Is that in the code or on github?

@Canato
Copy link
Member Author

Canato commented Oct 5, 2021

@Zikstar sadly the wiki I have to do my self. =///
(annoying part of the github not having the wiki in some folder in the code)

If you can just past here I will copy there.

@Zikstar
Copy link
Contributor

Zikstar commented Oct 5, 2021

Basically, the code snippets on this page have to be update to these, that's all

private val pickImage = registerForActivityResult(PickImageContract()) { 
    if(it != null) handleUri(it) else showError()
}

// starting to pick the image
private fun startPickImage() {
    pickImage.launch(
	PickImageContractOptions(includeCamera = false)
    )
}
private val pickImageCustom =
        registerForActivityResult(
            object : PickImageContract() {
                override fun parseResult(resultCode: Int, intent: Intent?): Uri? {
                    if (intent != null) {
                        context?.let {
                            context = null
                            return CropImage.getPickImageResultUriFilePath(it, intent).toUri()
                        }
                    }
                    context = null
                    return null
                }
            }
        ) { if(it != null) handleUri(it) else showError() }

// starting to pick the image
private fun startPickImageCustom() {
    pickImageCustom.launch(
	PickImageContractOptions(includeCamera = false)
    )
}

@Zikstar
Copy link
Contributor

Zikstar commented Oct 5, 2021

Finally, is it fair to say that "Gallery" is a subset of "Documents" based on this?
(The line about galleryIntent.type)
Screen Shot 2021-10-05 at 16 11 13
Because in that case my logic will be,

  • if both includeGallery and includeDocuments are false - add nothing
  • if includeDocuments is true, it doesn't matter what gallery is : add Documents
  • if includeDocuments is false, and includeGallery is true: add Gallery

@Canato
Copy link
Member Author

Canato commented Oct 6, 2021

I saw the PR before this comment, sorry.

Finally, is it fair to say that "Gallery" is a subset of "Documents" based on this?

We should not assume this if we can split 🤔 maybe using something more specific over */* but not sure.
If is to hard, let's make as subset and let is clear on the code somehow (documentation and name of the variables)

@Zikstar
Copy link
Contributor

Zikstar commented Oct 6, 2021

So, just looked into mime types, its not possible to use anything more specific, because the results would be irrelevant to images. The other specific mime types are video/, text/, audio/. It has to be a subset. And I can reflect that in the variable names("includeAllDocuments and includeOnlyGallery"). And maybe the documentation can reflect that if you set includeAllDocuments to true, you don't need to set includeOnlyGallery. Another option would be to remove "includeAllDocuments" completely, because any relevant applications will always be caught by image/ anyway(Including file apps, I tested this). And includeDocuments is always set to false in the library anyway

@Canato
Copy link
Member Author

Canato commented Oct 6, 2021

You have a fair point, I would go for the simple, let's remove documents

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

Successfully merging a pull request may close this issue.

3 participants