[Android] Add option to enable/disable download notification bubble #16556

tapanmodh · 2023-01-06T07:16:03Z

Resolves brave/brave-browser#25611

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:

Update the "ask where to save files" copy to: "Ask where to save files (if SD card is detected)"
Show download progress notifications with On/Off switch in download settings
Enable the switch, it should show notification bubble when downloading file
Disable the switch, it shouldn't show notification bubble when downloading file

github-actions · 2023-01-06T07:16:18Z

⚠️ PR head is an unsigned commit
commit: 16d0602dd80602e5bc7bc2568e0b3dd81681916b
reason: unsigned
Please follow the handbook to configure commit signing
cc: @tapanmodh

github-actions · 2023-01-06T07:16:18Z

⚠️ PR head is an unsigned commit
commit: 16d0602dd80602e5bc7bc2568e0b3dd81681916b
reason: unsigned
Please follow the handbook to configure commit signing
cc: @tapanmodh

github-actions · 2023-01-06T07:16:18Z

⚠️ PR head is an unsigned commit
commit: 16d0602dd80602e5bc7bc2568e0b3dd81681916b
reason: unsigned
Please follow the handbook to configure commit signing
cc: @tapanmodh

wchen342

Android test change++

android/java/org/chromium/chrome/browser/download/settings/BraveDownloadSettings.java

samartnik · 2023-01-06T13:47:12Z

android/java/org/chromium/chrome/browser/preferences/BravePreferenceKeys.java

@@ -25,6 +25,11 @@ public final class BravePreferenceKeys {
            "org.chromium.chrome.browser.upgrade.Milliseconds_New";
    public static final String BRAVE_DOWNLOADS_AUTOMATICALLY_OPEN_WHEN_POSSIBLE =
            "org.chromium.chrome.browser.downloads.Automatically_Open_When_Possible";
+
+    // Value is being used in
+    // browser/download/internal/android/java/src/org/chromium/chrome/browser/download/BraveDownloadMessageUiControllerImpl.java


We should move BravePreferenceKeys.java to base to be accessible in all components, like we did with BraveFeatureList.java

android/java/apk_for_test.flags

samartnik · 2023-01-06T13:50:58Z

android/java/proguard.flags

@@ -13,6 +13,10 @@
    *** createViewProvider(...);
 }

+-keep class org.chromium.chrome.browser.download.DownloadMessageUiControllerImpl {


This rule should be required for tests only, why we need it in the main proguard file?

My mistake. I'll remove it here and will add in tests

samartnik

lgtm

deeppandya · 2023-01-09T09:38:21Z

android/java/org/chromium/chrome/browser/download/settings/BraveDownloadSettings.java

@@ -35,6 +39,17 @@ public void onCreate(Bundle savedInstanceState) {
                (ChromeSwitchPreference) findPreference(PREF_AUTOMATICALLY_OPEN_WHEN_POSSIBLE);
        mAutomaticallyOpenWhenPossiblePref.setOnPreferenceChangeListener(this);

+        mDownloadProgressNotificationBubblePref =
+                (ChromeSwitchPreference) findPreference(PREF_DOWNLOAD_PROGRESS_NOTIFICATION_BUBBLE);


should we check for null here ?

I don't think it is needed as we are adding this ChromeSwitchPreference from our brave_download_preferences.xml

deeppandya · 2023-01-09T09:38:52Z

android/java/org/chromium/chrome/browser/download/settings/BraveDownloadSettings.java

@@ -66,6 +87,13 @@ public boolean onPreferenceChange(Preference preference, Object newValue) {
                            BravePreferenceKeys.BRAVE_DOWNLOADS_AUTOMATICALLY_OPEN_WHEN_POSSIBLE,
                            (boolean) newValue)
                    .apply();
+        } else if (PREF_DOWNLOAD_PROGRESS_NOTIFICATION_BUBBLE.equals(preference.getKey())) {
+            ContextUtils.getAppSharedPreferences()


can we use SharedPreferenceManager here ?

Not sure about this. we are already using ContextUtils.getAppSharedPreferences() in download setting's another item

deeppandya · 2023-01-09T09:40:36Z

android/java/org/chromium/base/BravePreferenceKeys.java


-package org.chromium.chrome.browser.preferences;


What was the reason to change package ?

We wants to access BravePreferenceKeys in browser/download/internal/android/java/src/org/chromium/chrome/browser/download/BraveDownloadMessageUiControllerImpl.java and from @samartnik's suggestion #16556 (comment)

deeppandya

LGTM

github-actions · 2023-01-11T10:43:55Z

⚠️ PR head is an unsigned commit
commit: 649dea5
reason: unsigned
Please follow the handbook to configure commit signing
cc: @tapanmodh

tapanmodh 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 Jan 6, 2023

tapanmodh added this to the 1.49.x - Nightly milestone Jan 6, 2023

tapanmodh requested review from deeppandya, wchen342 and samartnik January 6, 2023 07:16

tapanmodh requested a review from a team as a code owner January 6, 2023 07:16

tapanmodh self-assigned this Jan 6, 2023

tapanmodh requested a review from a team as a code owner January 6, 2023 12:24

wchen342 approved these changes Jan 6, 2023

View reviewed changes

tapanmodh force-pushed the enable_disable_download_progress_notification_android branch 2 times, most recently from d5a239e to 58f60b7 Compare January 6, 2023 13:11

samartnik requested changes Jan 6, 2023

View reviewed changes

Add option to enable/disable download notification bubble

649dea5

tapanmodh force-pushed the enable_disable_download_progress_notification_android branch from ec5b4cc to 649dea5 Compare January 6, 2023 16:19

tapanmodh requested a review from samartnik January 6, 2023 16:56

samartnik approved these changes Jan 6, 2023

View reviewed changes

deeppandya reviewed Jan 9, 2023

View reviewed changes

deeppandya approved these changes Jan 11, 2023

View reviewed changes

tapanmodh merged commit 98a6382 into master Jan 11, 2023

tapanmodh deleted the enable_disable_download_progress_notification_android branch January 11, 2023 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Add option to enable/disable download notification bubble #16556

[Android] Add option to enable/disable download notification bubble #16556

tapanmodh commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

wchen342 left a comment

samartnik Jan 6, 2023

samartnik Jan 6, 2023

tapanmodh Jan 6, 2023

samartnik left a comment

deeppandya Jan 9, 2023

tapanmodh Jan 9, 2023

deeppandya Jan 9, 2023

tapanmodh Jan 9, 2023

deeppandya Jan 9, 2023

tapanmodh Jan 9, 2023

deeppandya left a comment

github-actions bot commented Jan 11, 2023

[Android] Add option to enable/disable download notification bubble #16556

[Android] Add option to enable/disable download notification bubble #16556

Conversation

tapanmodh commented Jan 6, 2023

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

github-actions bot commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

wchen342 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samartnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeppandya left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2023