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

Insert Pictures https://github.com/autyzm-pg/friendly-plans/issues/145 #189

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Shreya001
Copy link

No description provided.

@Shreya001
Copy link
Author

please can someone tell me whats the issue?

Copy link
Collaborator

@malsolec malsolec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Looks really impressive! We really appreciate your help, great job. Thank you! 👍

The build is failing because I also can't build application locally. With merge to your code come some issue that are breaking build.

I have some ideas:

If you have any problems please ask, we really appreciate your effort.

* Created by shreya on 24/10/17.
*/

public class Utils {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename it to PremissionService

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one more class named this way.

public static boolean checkPermission(final Context context)
{
int currentAPIVersion = Build.VERSION.SDK_INT;
if(currentAPIVersion>=android.os.Build.VERSION_CODES.M)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding dots in code, import this constants.

import android.os.Build.VERSION_CODES;
 ....
VERSION_CODES.M

ActivityCompat.requestPermissions((Activity) context, new String[]{Manifest.permission.READ_EXTERNAL_STORAGE}, MY_PERMISSIONS_REQUEST_READ_EXTERNAL_STORAGE);
}
return false;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this 'elses' are required?

if (grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED) {
if(userChoosenTask.equals("Take Photo"))
cameraIntent();
else if(userChoosenTask.equals("Choose from Library"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be worth to change strings in userChoosenTask to be enum. Maybe enum with { CAMERA , GALLERY }

else if(userChoosenTask.equals("Choose from Library"))
galleryIntent();
} else {
//code for deny
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for that moment else can be removed.

private void galleryIntent()
{
Intent intent = new Intent();
intent.setType("image/*");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extract strings to final static.

fo.write(bytes.toByteArray());
fo.close();
} catch (FileNotFoundException e) {
e.printStackTrace();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is good to show some alert, go back to previous state, etc. if something went wrong.

}
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleAssetSelecting is not used now, but it was responsible for storing asset path in db.

{
if (ContextCompat.checkSelfPermission(context, Manifest.permission.READ_EXTERNAL_STORAGE) != PackageManager.PERMISSION_GRANTED) {
if (ActivityCompat.shouldShowRequestPermissionRationale((Activity) context, Manifest.permission.READ_EXTERNAL_STORAGE)) {
AlertDialog.Builder alertBuilder = new AlertDialog.Builder(context);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe extract this dialog builder to ( line 32 - 43 ) to separated private method name what this is doing -> showPermissionAlert() ?

public void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);

if (resultCode == Activity.RESULT_OK) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if your ifs match our convention ->

 if () { 
...
}

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

Successfully merging this pull request may close these issues.

None yet

2 participants