Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Added download count/badge and progress (closes #3023) #9864

Merged
merged 1 commit into from
Jul 29, 2017

Conversation

timborden
Copy link
Contributor

@timborden timborden commented Jul 5, 2017

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Closes #3023

Test Plan:

  • I tested manually. There didn't appear to be any unit tests for /app/browser/updateElectronDownloadItem.js

    1. Go to https://brave.com/downloads.html
    2. Download one of the builds
    3. Notice the taskbar icon has a progress bar. If macOS, a badge is shown with # of concurrent downloads
  • I can add tests that check the badge count (app.getBadgeCount), if you think it's necessary...although it feels like we're testing electron code with that

  • I can write unit tests for the progress calculations, if you think it's necessary

Notes:

  • I'm not sure I like having to pass win into the updateDownloadState and updateElectronDownloadItem functions....but there didn't seem to be a better way. If you've got a better approach...I'm happy to refactor.

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@NejcZdovc NejcZdovc added this to the 0.20.x (Nightly Channel) milestone Jul 5, 2017
@bsclifton
Copy link
Member

@timborden sorry for the super long turn-around... finally trying this out now 😄

@bsclifton bsclifton force-pushed the feature/download-notifications branch from 5f267ea to 85065fa Compare July 28, 2017 22:52
@bsclifton
Copy link
Member

bsclifton commented Jul 28, 2017

Here's what it looks like on macOS:
progress-macos

It also does properly track # of downloads:
screen shot 2017-07-28 at 3 55 59 pm

@codecov-io
Copy link

Codecov Report

Merging #9864 into master will decrease coverage by 0.05%.
The diff coverage is 23.33%.

@@            Coverage Diff             @@
##           master    #9864      +/-   ##
==========================================
- Coverage   52.81%   52.76%   -0.06%     
==========================================
  Files         227      227              
  Lines       20192    20218      +26     
  Branches     3233     3236       +3     
==========================================
+ Hits        10665    10668       +3     
- Misses       9527     9550      +23
Flag Coverage Δ
#unittest 52.76% <23.33%> (-0.06%) ⬇️
Impacted Files Coverage Δ
app/filtering.js 18.33% <0%> (ø) ⬆️
app/browser/electronDownloadItem.js 42.55% <26.92%> (-24.12%) ⬇️
js/stores/appStoreRenderer.js 91.17% <0%> (-8.83%) ⬇️

@bsclifton
Copy link
Member

bsclifton commented Jul 28, 2017

Here's how it looks on Windows:
progress-windows

Unlike macOS, Windows doesn't have a badge with # of downloads. Looks good though! I did notice that after the download is complete, the icon in the taskbar stays green (like 100%) forever.

@timborden can you check that out? I checked Chrome on Windows and the progress goes away once the download hits 100%. We want to do that also
edit: I tried again and it works great

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

I just re-tried and it worked great, as expected 😄 Great job on this one!

@bsclifton
Copy link
Member

NOTE: I tried on Linux (using Ubuntu 14 with Gnome 3) and I didn't see the progress bar or any badges (it just behaved regularly).

@bsclifton bsclifton merged commit e4713aa into brave:master Jul 29, 2017
@bsclifton
Copy link
Member

Congrats on your first contribution, @timborden 😄 👍 (and huge thanks!)

bsclifton added a commit that referenced this pull request Jul 29, 2017
Added download count/badge and progress (closes #3023)
@luixxiul
Copy link
Contributor

@timborden It looks great! Looking forward to see it on board :-)

@timborden
Copy link
Contributor Author

Thanks @bsclifton and @luixxiul!

(just spotted that this was merged...sorry for not picking up the earlier messages....adjusting GitHub settings now)

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

Successfully merging this pull request may close these issues.

5 participants