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

Unify Notifications #12907

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

Unify Notifications #12907

wants to merge 27 commits into from

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Apr 19, 2024 •
edited

  • Tests written, or not not needed

What does this PR?

  • Unify notifications for downloads and uploads to display all progress in a single notification rather than multiple notifications.
  • Make download and upload notifications dismissible to prevent stuck notifications on some devices.
  • Prevent unnecessary notification updates by adding debounce for the progress updates of downloads and uploads.
  • Use a parent class for Upload and Download Notification Manager to prevent code duplication.
  • Remove unnecessary usage of notification dismiss.
  • Add BigTextStyle for upload and download notifications to provide better visibility
  • Fix notification blinking for uploads

Before

before.mp4

After

v.mp4

How to Test

  • Test one file download

  • Test one file upload

  • Test multiple file upload

  • Test multiple file download

  • Test all above scenarios for e2e

@alperozturk96 alperozturk96 linked an issue Apr 19, 2024 that may be closed by this pull request
Unified Notifications #12256
Open
@alperozturk96 alperozturk96 force-pushed the feature/unify-notifications branch 2 times, most recently from 46bf27c to 32d0efb Compare April 26, 2024 14:05
@BluePixel4k BluePixel4k mentioned this pull request Apr 29, 2024
Open
@alperozturk96 alperozturk96 force-pushed the feature/unify-notifications branch 4 times, most recently from fb239e5 to 248ee8a Compare May 3, 2024 06:57
ZetaTom
ZetaTom previously requested changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@ZetaTom ZetaTom left a comment

Choose a reason for hiding this comment

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

In general these changes seem to work and not break anything.

Please revert any changes made to app/src/main/res/values-*/strings.xml as these files are managed via transifex.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadWorker.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nextcloud/client/jobs/upload/FileUploadWorker.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nextcloud/client/jobs/upload/UploadNotificationManager.kt Outdated Show resolved Hide resolved
app/src/main/java/com/nextcloud/client/jobs/upload/UploadNotificationManager.kt Show resolved Hide resolved
@alperozturk96 alperozturk96 marked this pull request as draft May 8, 2024 13:11
auto-merge was automatically disabled May 8, 2024 13:11

Pull request was converted to draft

@alperozturk96
Make upload notifications dismissable
e15282e 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Use one notification for uploads
20dcf65 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Use one notification for uploads
92ff05a 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Use one notification for uploads
3cceb9a 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Use one notification for uploads
0687107 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Fix notification sync issue
9a410a9 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Simplify download notifications
cc4ecab 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
alperozturk96 and others added 19 commits May 31, 2024 16:31
@alperozturk96
Use one notification for all downloads
33dc3a2 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Fix code analytics
e638805 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Remove unused resources
b292716 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Remove unused resources
a378286 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Remove unnecessary dismiss
c581f43 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96 @ZetaTom
Update app/src/main/java/com/nextcloud/client/jobs/upload/UploadNotif…
5bd009a 
…icationManager.kt

Co-authored-by: Tom <70907959+ZetaTom@users.noreply.github.com>
Signed-off-by: Alper Öztürk <67455295+alperozturk96@users.noreply.github.com>
@alperozturk96
Add getTotalUploadSize and inject limit to getUploadPage
0871737 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Make upload_notification_manager_start_text translatable
a7d3535 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Use parent class for common logic WorkerNotificationManager
8b94974 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Extract common logic
11c3de2 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Remove deprecated usage of NotificationCompat.Builder
de0b3de 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Extract setProgress common logic
796a934 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Extract dismissNotification common logic
1d8f308 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Use BigTextStyle
286445c 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Add minProgressUpdateInterval time for upload and downloads for preve…
a31867f 
…nting unnecessary updates

Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Only show total download/upload progress if total count bigger than 0
161e9f8 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Fix code analytics
c2a788e 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Delete unused call
a25c355 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Remove duplicated upload conditions to fix code analysis
ceac70c 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96
Fix upload notification blinking
a76bd7f 
Signed-off-by: alperozturk <alper_ozturk@proton.me>
@alperozturk96 alperozturk96 marked this pull request as ready for review May 31, 2024 15:14
@alperozturk96 alperozturk96 dismissed ZetaTom’s stale review May 31, 2024 15:14

Not valid anymore

@github-actions GitHub Actions
Copy link

Codacy

Lint

TypemasterPR
Warnings7070
Errors33

SpotBugs

CategoryBaseNew
Bad practice6464
Correctness6969
Dodgy code346346
Experimental11
Internationalization77
Malicious code vulnerability22
Multithreaded correctness66
Performance5757
Security1919
Total571571

@github-actions GitHub Actions
Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12907.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@github-actions GitHub Actions
Copy link

blue-Light-Screenshot test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/12907-Screenshot-blue-Light-15-31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasKaminsky tobiasKaminsky Awaiting requested review from tobiasKaminsky

@JonasMayerDev JonasMayerDev Awaiting requested review from JonasMayerDev

@AndyScherzinger AndyScherzinger Awaiting requested review from AndyScherzinger

@ZetaTom ZetaTom Awaiting requested review from ZetaTom

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
3. to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unified Notifications
3 participants
@alperozturk96 @AndyScherzinger @ZetaTom