Dialog buttons set as opposite to Android design conventions

[aryan-earth]

Summary:
The text of positive and negative buttons of the dialogs in DialogUtils has been set as the opposite and should we swapped. The positive button text is set as "No" and the negative button text is set as "Yes" as evident in the image attached below.

Side notes:
This issue was created from a discussion that happened at #3397. The relevant discussion starts from #3397 (comment). This was created to discuss the rationale behind why the buttons are the way they are now, if there are any reasons.

Device and Android version:
Redmi Note 5 Pro & Android 9

Commons app version:
2.12.3-debug-testing~65592539b

Screen-shots:

Current state

Should be like the image below:

Would you like to work on the issue?

Sure. Although, if a new volunteer wants to get their hands dirty in the commons code, this could be a great opportunity for them. This could be marked as a Good First Issue.

[madhurgupta10]

@glitch101 I am not sure about the design convention but I think you are right, even in Gmail or Android Settings the convention seems to place the Cancel on the Left and OK on Right.

@sivaraam What do you think?

[sivaraam]

Yeah, its a common convention indeed. That said we generally don't have to worry about the position of the buttons. If we just specify the positive and negative actions correctly, that should do. Here positive actions corresponds to "Confirming actions" and negative action corresponds to the "Dismissive actions" in the material design guideline.

Just an FYI, this issue was created from a discussion that happened at #3397 . The relevant discussion starts from #3397 (comment). This was created to discuss the rationale behind why the buttons are the way they are now, if there are any reasons.

[madhurgupta10]

@glitch101 Do tag the comment or issue in your description, it will accelerate the discussion.

[aryan-earth]

Just an FYI, this issue was created from a discussion that happened at #3397 . The relevant discussion starts from #3397 (comment). This was created to discuss the rationale behind why the buttons are the way they are now, if there are any reasons.

Can I put this subsection of your comment in the description?

[aryan-earth]

@glitch101 Do tag the comment or issue in your description, it will accelerate the discussion.

Thanks for the tip. I will do that :)

[sivaraam]

Can I put this subsection of your comment in the description?

Sure. Here's the non-rendered markdown text of that part:

Just an FYI, this issue was created from a discussion that happened at https://github.com/commons-app/apps-android-commons/issues/3397 . The relevant discussion starts from https://github.com/commons-app/apps-android-commons/issues/3397#issuecomment-589927150. This was created to [discuss _the rationale behind why_ the buttons](https://github.com/commons-app/apps-android-commons/issues/3397#issuecomment-590108849) are the way they are now, if there are any reasons.

[misaochan]

Wow, #3397 is a really long discussion thread... I'm still halfway through it, but for this issue I think it is safe to say that it is OK to swap the buttons as per common conventions.

[aryan-earth]

Wow, #3397 is a really long discussion thread... I'm still halfway through it, but for this issue I think it is safe to say that it is OK to swap the buttons as per common conventions.

Yes, #3397 contains quite a lot of unrelated discussion with respect to this issue. I have updated the issue description to include the links to relevant discussion on #3397 thread.

[sivaraam]

Wow, #3397 is a really long discussion thread.

Yeah. Sorry about that! 😅

... but for this issue I think it is safe to say that it is OK to swap the buttons as per common conventions.

If we're ok with swapping the "Yes" and "No" in showAlertDialog(Activity, String, String, Runnable, Runnable onNegativeBtnClick, View, boolean), I think there are at least two usages of that method which we have to update to ensure the "Yes" remains as the negative action. Yeah, you read that right I did say negative action. I realize it's common to have "Yes" as the positive action but in the following cases I strongly believe it's more appropriate to have "Yes" as the negative action:

• UploadMediaDetailFragment.java#L352
• UploadMediaDetailFragment.java#L370

[FawziyahAlebiosu]

This is definitely an interesting issue, @glitch101 can you assign this to me?

[aryan-earth]

This is definitely an interesting issue, @glitch101 can you assign this to me?

Sorry, I cannot assign an issue to you. I do not have write access to this repository. I am a contributor. I am tagging one of the maintainers so that they can assign you. @maskaravivek

[sivaraam]

@FawziyahAlebiosu You can take this up. I've assigned this to you. Be sure to let us know how you get on :)

[misaochan]

I think there are at least two usages of that method which we have to update to ensure the "Yes" remains as the negative action. Yeah, you read that right I did say negative action. I realize it's common to have "Yes" as the positive action but in the following cases I strongly believe it's more appropriate to have "Yes" as the negative action

Those are the warnings when a user tries to upload a duplicate or "bad" picture, right? I agree, those should be kept with "Yes" as the negative action. But the dialog button mentioned in this issue (for turning on location) can be changed.

[macgills]

<string name="upload_title_duplicate" formatted="true">A file with the file name %1$s exists. Are you sure you want to proceed?</string>

That to me seems like yes should be in the positive position.

<string name="upload_problem_do_you_continue">Do you still want to upload this picture?</string>

That also seems like a yes in positive.

Positive/negative have no correlation between destructive/constructive actions. It is simply the response to the question posed

[sivaraam]

Those are the warnings when a user tries to upload a duplicate or "bad" picture, right?

Yeah

Positive/negative have no correlation between destructive/constructive actions. It is simply the response to the question posed

That's a good point. They really don't seem to have any correlation. FWIW, even the material design guideline for dialog boxes is against keeping dismissive actions (in our case "No") in the left of the dialog. Looks like I was wrong in thinking they are correlated 🤔 Thanks for correcting that @macgills 🙂

One other reason I was suggesting to keep the "Yes" as negative action as I thought that it was intentionally decided to kept them that way. Now that I think about it seems the decision might've been made with the assumption that there's a direct correlation between positive/negative and destructive/constructive actions. @maskaravivek might be able to shed some light about this.

[macgills]

Also if we go with the material guidelines "Yes/No" are poor words to use, the action should be repeated in the positive button eg "Upload/Proceed" and the negative should be something like "Cancel" if you cancel an operation

[FawziyahAlebiosu]

Also if we go with the material guidelines "Yes/No" are poor words to use, the action should be repeated in the positive button eg "Upload/Proceed" and the negative should be something like "Cancel" if you cancel an operation

Will be sure to replace it with that.

[FawziyahAlebiosu]

I think there are at least two usages of that method which we have to update to ensure the "Yes" remains as the negative action. Yeah, you read that right I did say negative action. I realize it's common to have "Yes" as the positive action but in the following cases I strongly believe it's more appropriate to have "Yes" as the negative action

Those are the warnings when a user tries to upload a duplicate or "bad" picture, right? I agree, those should be kept with "Yes" as the negative action. But the dialog button mentioned in this issue (for turning on location) can be changed.

So, is it safe to conclude that the swapping of positive and negative action, as well as replacing with Cancel, and Proceed, is ONLY to the dialog box that shows up when turning location, correct? I am not doing this to any other dialog boxes, right?

[sivaraam]

So, is it safe to conclude that the swapping of positive and negative action, as well as replacing with Cancel, and Proceed, is ONLY to the dialog box that shows up when turning location, correct?

There seems to be some confusion here. Let me clarify.

@FawziyahAlebiosu You don't have to worry about the dialog that shows up when turning on location. It's just now being added in PR #3438. As of now, the only changes you would have to do is to swap the negative and positive button text in the DialogUtil#showAlertDialog method.

Changing the text of the buttons actually not in the scope of this issue. So, if you're interested in doing that change you can you can do that separately. I've created #3494 for that 🙂

Others, correct me if I'm wrong somewhere.

[FawziyahAlebiosu]

Ahh @sivaraam thanks for clarifying haha! I've created a pull request to address the correct issue then. Also, I'll definitely work on issue #3494 as well.

[FawziyahAlebiosu]

Update while testing this, I realized that I will need to figure out which part in the method I need to swap as well-so I'm halfway done!

[macgills]

@FawziyahAlebiosu I would giving the official guide a read if you are not too familiar with Dialogs, that should familiarise you with their api

[macgills]

Due to an error I am reopening to verify the usages of DialogUtil

 public static void showAlertDialog(Activity activity,
                                       String title,
                                       String message,
                                       final Runnable onPositiveBtnClick,
                                       final Runnable onNegativeBtnClick)

Has 3 usages, its api passes yes & no to the correct positions.

 DialogUtil.showAlertDialog(getActivity(),
                getString(R.string.upload_nearby_place_found_title),
                String.format(Locale.getDefault(),
                        getString(R.string.upload_nearby_place_found_description),
                        place.getName()),
                () -> {
                    etTitle.setText(place.getName());
                    Description description = new Description();
                    description.setLanguageCode("en");
                    description.setDescriptionText(place.getLongDescription());
                    descriptions = Arrays.asList(description);
                    setDescriptionsInAdapter(descriptions);
                },
                () -> {

                });
    }

Positive click listener in correct first position.

        DialogUtil.showAlertDialog(getActivity(),
                getString(R.string.warning),
                String.format(Locale.getDefault(),
                        uploadTitleFormat,
                        uploadItem.getFileName()),
                () -> {
                    uploadItem.setImageQuality(ImageUtils.IMAGE_KEEP);
                    onNextButtonClicked();
                }, null);

Positive click listener in correct first position.

            DialogUtil.showAlertDialog(getActivity(),
                    getString(R.string.warning),
                    errorMessageForResult,
                    () -> {
                        uploadItem.setImageQuality(ImageUtils.IMAGE_KEEP);
                        onNextButtonClicked();
                    },
                    () -> deleteThisPicture()
        );

Positive click listener in correct first position

    public static void showAlertDialog(Activity activity,
                                       String title,
                                       String message,
                                       String positiveButtonText,
                                       String negativeButtonText,
                                       final Runnable onPositiveBtnClick,
                                       final Runnable onNegativeBtnClick) {

This method has 8 usages

        DialogUtil
            .showAlertDialog(getActivity(), getString(R.string.ask_to_turn_location_on), getString(R.string.nearby_needs_location),
                getString(R.string.yes), getString(R.string.no),  this::openLocationSettings, null);

yes,no and click listeners in correct position.

        DialogUtil.showAlertDialog(activity,
                activity.getString(R.string.quiz),
                activity.getString(R.string.quiz_alert_message, REVERT_PERCENTAGE_FOR_MESSAGE),
                activity.getString(R.string.about_translate_proceed),
                activity.getString(android.R.string.cancel),
                () -> startQuizActivity(activity),
                null);

proceed,cancel positivie click in correct position.

      DialogUtil.showAlertDialog(ReviewActivity.this,
                getString(R.string.skip_image).toUpperCase(),
                getString(R.string.skip_image_explanation),
                getString(android.R.string.ok),
                "",
                null,
                null);

ok in correct position, no click listeners

        DialogUtil.showAlertDialog(ReviewActivity.this,
                getString(R.string.title_activity_review),
                getString(R.string.review_image_explanation),
                getString(android.R.string.ok),
                "",
                null,
                null);

ok in correct position, no click listeners.

        DialogUtil.showAlertDialog(getActivity(),
                getString(R.string.no_categories_selected),
                getString(R.string.no_categories_selected_warning_desc),
                getString(R.string.yes_submit),
                getString(R.string.no_go_back),
                () -> goToNextScreen(),
                null);

yes,no,positive correct.

        showAlertDialog(activity,
                title,
                message,
                activity.getString(R.string.yes),
                activity.getString(R.string.no),
                onPositiveBtnClick,
                onNegativeBtnClick);

all correct positions.

                        DialogUtil.showAlertDialog(activity, activity.getString(rationaleTitle),
                            activity.getString(rationaleMessage),
                            activity.getString(R.string.navigation_item_settings), null,
                            () -> askUserToManuallyEnablePermissionFromSettings(activity), null);

positive button and listener in correct positions.

                    DialogUtil.showAlertDialog(activity, activity.getString(rationaleTitle),
                        activity.getString(rationaleMessage),
                        activity.getString(android.R.string.ok),
                        activity.getString(android.R.string.cancel),
                        token::continuePermissionRequest, token::cancelPermissionRequest);

positive/negative message/liteners in correct positions

 public static void showAlertDialog(Activity activity,
                                       String title,
                                       String message,
                                       final Runnable onPositiveBtnClick,
                                       final Runnable onNegativeBtnClick,
                                       View customView,
                                       boolean cancelable) {

1 usage

        DialogUtil.showAlertDialog(getActivity(),
                getString(R.string.nearby_card_permission_title),
                getString(R.string.nearby_card_permission_explanation),
                this::requestLocationPermission,
                this::displayYouWontSeeNearbyMessage,
                checkBoxView,
                false);

pos/neg click in correct position.

    public static void showAlertDialog(Activity activity, String title, String message, String positiveButtonText, final Runnable positiveButtonClick, boolean cancellable) {

1 usage

        DialogUtil.showAlertDialog(getActivity(), getString(titleStringID), getString(messageStringId), getString(android.R.string.ok), null, true);

no click listener to worry about.

That is all the dialogs.

Conclusion: there are no further erroneous usages of DialogUtil