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.

Sign up for GitHub

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

Jump to bottom

Improve brave rating dialog #16425

Merged
merged 1 commit into from
Jan 3, 2023
Merged

Improve brave rating dialog #16425

merged 1 commit into from
Jan 3, 2023

Conversation

sujitacharya2005
Copy link
Contributor

@sujitacharya2005 sujitacharya2005 commented Dec 20, 2022 •
edited
Loading

Resolves brave/brave-browser#25777

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Show dialog in launch time OR show ui in news screen based on following condition.

  1. 5th launch or more.
  2. 4 days of use in the last 7 days. Does not have to be consecutive.

Any one

  1. User has added at least 5 bookmarks.

  2. User has set Brave as default.

  3. User has paid for the VPN subscription.

  4. Then show the dialog every 30 days.

news smiley Thanks feedback Ask for rating Google rating dialog Launch Screen
Screenshot_20221228_090721 Screenshot_20221228_085936 Screenshot_20221228_090004 Screenshot_20221228_090041 Screenshot_20221228_090107 Screenshot_20221228_090643
Screenshot_20221228_091136 Screenshot_20221228_091209 Screenshot_20221228_091230 Screenshot_20221228_091252 Screenshot_20221228_091322 Screenshot_20221228_091104

@sujitacharya2005 sujitacharya2005 added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Dec 20, 2022
@sujitacharya2005 sujitacharya2005 force-pushed the 25777_rating_prompt_update2 branch 3 times, most recently from 41cdfa4 to 3ce8562 Compare December 21, 2022 09:11
@github-actions
Copy link
Contributor

⚠️ PR head is an unsigned commit
commit: 3ce8562680441f960b727dec2752926090b6a8bf
reason: unsigned
Please follow the handbook to configure commit signing
cc: @sujitacharya2005

@sujitacharya2005 sujitacharya2005 force-pushed the 25777_rating_prompt_update2 branch 2 times, most recently from 0517b8a to 1f31885 Compare December 27, 2022 17:55
samartnik
samartnik requested changes Dec 28, 2022
View reviewed changes
android/java/org/chromium/chrome/browser/brave_news/CardBuilderFeedCard.java Outdated
private void showBraveNewsRatingUI(
LinearLayout linearLayout, RecyclerView.LayoutParams linearLayoutParams) {
/*
Image Title
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what this comment helps with? To me it is rather confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

android/java/org/chromium/chrome/browser/brave_news/CardBuilderFeedCard.java Outdated

private void showBraveRateDialog() {
BraveRateDialogFragment mRateDialogFragment = new BraveRateDialogFragment();
mRateDialogFragment.show(((ChromeActivity) mActivity).getSupportFragmentManager(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check if mActivity is actually ChromeActivity as we may get null pointer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

android/java/org/chromium/chrome/browser/rate/BraveAskPlayStoreRatingDialog.java Outdated
transaction.add(this, tag);
transaction.commitAllowingStateLoss();
} catch (IllegalStateException e) {
Log.e("BraveRateDialogFragment", e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use TAG variable with logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

android/java/org/chromium/chrome/browser/rate/BraveAskPlayStoreRatingDialog.java Show resolved Hide resolved
android/java/org/chromium/chrome/browser/rate/BraveRateDialogFragment.java Outdated
transaction.add(this, tag);
transaction.commitAllowingStateLoss();
} catch (IllegalStateException e) {
Log.e("BraveRateDialogFragment", e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use TAG variable with logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

android/java/org/chromium/chrome/browser/rate/BraveRateThanksFeedbackDialog.java Outdated

public class BraveRateThanksFeedbackDialog extends BottomSheetDialogFragment {
final public static String TAG_FRAGMENT = "brave_rate_thanks_feedback_dialog_tag";
private static final String TAG = "ask_play_store";
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't see where it is used, we should probably use it with logs, but name of the TAG should be different then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

android/java/org/chromium/chrome/browser/rate/BraveRateThanksFeedbackDialog.java Outdated
transaction.add(this, tag);
transaction.commitAllowingStateLoss();
} catch (IllegalStateException e) {
Log.e("BraveRateDialogFragment", e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use TAG variable with logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

browser/ui/android/strings/android_brave_strings.grd Show resolved Hide resolved
@sujitacharya2005 sujitacharya2005 force-pushed the 25777_rating_prompt_update2 branch 3 times, most recently from 182c711 to 5ac426a Compare December 28, 2022 16:47
SergeyZhukovsky
SergeyZhukovsky approved these changes Dec 29, 2022
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

samartnik
samartnik approved these changes Dec 29, 2022
View reviewed changes
Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

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

lgtm

deeppandya
deeppandya reviewed Dec 29, 2022
View reviewed changes
android/java/org/chromium/chrome/browser/brave_news/CardBuilderFeedCard.java Outdated
@@ -639,6 +647,7 @@ public void onClick(View v) {

*/
mLinearLayout.setOrientation(LinearLayout.HORIZONTAL);

Copy link
Contributor

Choose a reason for hiding this comment

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

nit : remove new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

android/java/org/chromium/chrome/browser/brave_news/CardBuilderFeedCard.java Outdated
View view = LayoutInflater.from(ContextUtils.getApplicationContext())
.inflate(R.layout.brave_rating_news_layout, null);
view.setOnClickListener((v) -> { showBraveRateDialog(); });

Copy link
Contributor

@deeppandya deeppandya Dec 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit : remove new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

android/java/org/chromium/chrome/browser/rate/BraveAskPlayStoreRatingDialog.java
@Override
public void onCreate(@Nullable Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setStyle(STYLE_NORMAL, R.style.AppBottomSheetDialogTheme);
Copy link
Contributor

Choose a reason for hiding this comment

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

@sujitacharya2005 have you verified the change on android 6 ? i want to make sure for style on android 6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same style is already used in BraveStatsBottomSheetDialogFragment.java.
I will re-verify it.

android/java/org/chromium/chrome/browser/rate/RateUtils.java

private static final String PREF_LAST_SESSION_SHOWN = "last_session_shown";

private static final String PREF_LAST_TIME_APP_USED_DATE1 = "last_time_app_used_date1";
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using 4 different pref, can we use last open timestamp and compare with the current timestamp and determine if it's consecutive days ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here current timeStamp need to check last 3 timeStamp, if all are less than 7 means , 4 days used in last seven days.
For that need to keep 4 different timestamp in pref.

android/java/org/chromium/chrome/browser/rate/RateUtils.java
sharedPreferencesEditor.apply();
}

public boolean shouldShowRateDialog() {
int appOpenCount = SharedPreferencesManager.getInstance().readInt(BravePreferenceKeys.BRAVE_APP_OPEN_COUNT);
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

it's amazing to see the comments like this for the conditions.

android/java/res/drawable/brave_rating_rate_now_bg.xml
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

@sujitacharya2005 you can run npm run presubmit to see if we have license header related issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

it would complain about few missing license headers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved all presubmit error

android/java/res/layout/brave_ask_play_store_rating_dialog.xml
android:layout_width="match_parent"
android:layout_height="match_parent"
android:background="@drawable/rating_bottomsheet_background">

Copy link
Contributor

Choose a reason for hiding this comment

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

nit : extra new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@sujitacharya2005 sujitacharya2005 force-pushed the 25777_rating_prompt_update2 branch 2 times, most recently from a1680f2 to 617bef0 Compare December 30, 2022 06:49
@sujitacharya2005 sujitacharya2005 force-pushed the 25777_rating_prompt_update2 branch from 617bef0 to 83a29d6 Compare December 30, 2022 09:59
deeppandya
deeppandya approved these changes Jan 2, 2023
View reviewed changes
Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

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

Changes LGTM. Let's make sure the change in android 6

tapanmodh
tapanmodh approved these changes Jan 3, 2023
View reviewed changes
Copy link
Contributor

@tapanmodh tapanmodh left a comment

Choose a reason for hiding this comment

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

LGTM

@sujitacharya2005 sujitacharya2005 merged commit ee208d0 into master Jan 3, 2023
@sujitacharya2005 sujitacharya2005 deleted the 25777_rating_prompt_update2 branch January 3, 2023 11:42
@github-actions github-actions bot added this to the 1.48.x - Nightly milestone Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samartnik samartnik samartnik approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@tapanmodh tapanmodh tapanmodh approved these changes

@deeppandya deeppandya deeppandya approved these changes

Assignees

@sujitacharya2005 sujitacharya2005

Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
Milestone
1.48.x - Release
Development

Successfully merging this pull request may close these issues.

[Android] Improve Rating Prompts
5 participants
@sujitacharya2005 @deeppandya @tapanmodh @SergeyZhukovsky @samartnik