For #352: Delete a download #15577

kglazko · 2020-09-30T21:09:04Z

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt

eliserichards

Ok, whew. This is a big PR 😄 I think this is a great start. Let's focus on the simple case of deleting one item at a time and file a followup to handle multi-select. I think that will help us see the next steps more clearly

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadAdapter.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadController.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadItemMenu.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadView.kt

kglazko · 2020-10-15T22:26:25Z

Changes: Delete one, Delete Multi, and Delete All now work. Sometimes there is a weird issue with the UI not updating to show the disappeared downloads right away, need extra eyes to figure out why.

codecov-io · 2020-10-15T22:40:16Z

Codecov Report

Merging #15577 into master will decrease coverage by 0.12%.
The diff coverage is 11.86%.

@@             Coverage Diff              @@
##             master   #15577      +/-   ##
============================================
- Coverage     30.03%   29.91%   -0.13%     
- Complexity     1207     1213       +6     
============================================
  Files           453      454       +1     
  Lines         18516    18686     +170     
  Branches       2548     2571      +23     
============================================
+ Hits           5562     5590      +28     
- Misses        12522    12664     +142     
  Partials        432      432

Impacted Files	Coverage Δ	Complexity Δ
...ozilla/fenix/library/downloads/DownloadFragment.kt	`11.01% <0.00%> (-17.88%)`	`1.00 <0.00> (ø)`
...ozilla/fenix/library/downloads/DownloadItemMenu.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...rg/mozilla/fenix/library/downloads/DownloadView.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...wnloads/viewholders/DownloadsListItemViewHolder.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...mozilla/fenix/library/downloads/DownloadAdapter.kt	`40.90% <20.00%> (-12.94%)`	`4.00 <1.00> (+1.00)`	⬇️
...a/fenix/library/downloads/DownloadFragmentStore.kt	`53.48% <36.00%> (+11.38%)`	`1.00 <0.00> (+1.00)`
...illa/fenix/library/downloads/DownloadController.kt	`75.00% <100.00%> (+17.85%)`	`0.00 <0.00> (ø)`
...illa/fenix/library/downloads/DownloadInteractor.kt	`100.00% <100.00%> (+40.00%)`	`8.00 <5.00> (+5.00)`
...a/org/mozilla/fenix/browser/BaseBrowserFragment.kt	`7.25% <0.00%> (-0.70%)`	`8.00% <0.00%> (-1.00%)`
...c/main/java/org/mozilla/fenix/StrictModeManager.kt	`83.78% <0.00%> (ø)`	`7.00% <0.00%> (ø%)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a12d1a...d9d1be5. Read the comment docs.

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadAdapter.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadController.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadView.kt

...src/main/java/org/mozilla/fenix/library/downloads/viewholders/DownloadsListItemViewHolder.kt

eliserichards · 2020-10-16T17:41:33Z

...src/main/java/org/mozilla/fenix/library/downloads/viewholders/DownloadsListItemViewHolder.kt

        itemView.download_layout.titleView.text = item.fileName
        itemView.download_layout.urlView.text = item.size.toLong().toMegabyteOrKilobyteString()

+        toggleTopContent(showDeleteButton, mode === DownloadFragmentState.Mode.Normal)


= = = ? what does the triple = do :O

Not sure if this is necessary here (re: our convo earlier). Let's just leave this as-is for now.

Here you can find some docs around the equality operators, in summary === is comparing based on reference, vs == that compare based on equals, in this case either will work asDownloadFragmentState.Mode.Normal is an object it has only one reference (it's a singleton), I would prefer if we could use == as it will have the same effect and avoid the inherited complexity of ===.

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadAdapter.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadController.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragmentStore.kt

...src/main/java/org/mozilla/fenix/library/downloads/viewholders/DownloadsListItemViewHolder.kt

Amejia481 · 2020-10-20T18:24:03Z

@kglazko could we add some test to cover this new functionality? It will help to avoid future regression :)

Amejia481 · 2020-10-20T18:49:09Z

@kglazko are there any UI mockups ?

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt

eliserichards

Following up on our conversation with @boek and @Amejia481 earlier, our action items here are to:

add some comments/kdocs to these files explaining how the pending deletion functions (@kglazko)
file an issue for refactoring downloads, history, bookmarks, etc to improve clarity and code quality (@eliserichards)

eliserichards

There are a few nits from the last few rounds that still need to be cleaned up, but this is looking fantastic! Once those nits are taken care of I want to come back and do some manual testing (which I'll probably ask your help on)

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt

Amejia481

It looks good to me!
In a follow up we can increase the code coverage for this feature :)

There is a ktlint issue.

[task 2020-11-02T18:44:32.550Z] > Task :ktlint
[task 2020-11-02T18:44:32.550Z] /builds/worker/checkouts/src/app/src/main/java/org/mozilla/fenix/library/downloads/viewholders/DownloadsListItemViewHolder.kt:21:1: Unused import
[task 2020-11-02T18:44:34.849Z] 
[task 2020-11-02T18:44:34.850Z] > Task :ktlint FAILED
[task 2020-11-02T18:44:34.851Z] 
[task 2020-11-02T18:44:34.851Z] FAILURE: Build failed with an exception.
[task 2020-11-02T18:44:34.851Z]

eliserichards · 2020-11-04T17:44:24Z

Yayyyyy! This is ready to go! 🥇

kglazko added the pr:do-not-land label Sep 30, 2020

gabrielluong added pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on and removed pr:do-not-land labels Oct 1, 2020

Amejia481 self-requested a review October 1, 2020 14:03

Amejia481 self-assigned this Oct 1, 2020

Amejia481 reviewed Oct 1, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt Outdated Show resolved Hide resolved

Amejia481 mentioned this pull request Oct 1, 2020

Closes issue #8587: Add use cases for removing individual and all downloads mozilla-mobile/android-components#8588

Merged

4 tasks

kglazko force-pushed the PreserveViewDownloadsMenus branch from f5068e3 to c965f0d Compare October 6, 2020 20:33

kglazko requested a review from eliserichards October 6, 2020 20:34

eliserichards reviewed Oct 6, 2020

View reviewed changes

kglazko force-pushed the PreserveViewDownloadsMenus branch 2 times, most recently from 719ce28 to 7fc12ab Compare October 15, 2020 21:59

kglazko changed the title ~~WIP: Delete Downloads~~ For #352: Delete a download Oct 15, 2020

kglazko marked this pull request as ready for review October 15, 2020 22:16

kglazko requested review from a team as code owners October 15, 2020 22:16

kglazko requested review from eliserichards and Amejia481 October 15, 2020 22:25

eliserichards suggested changes Oct 16, 2020

View reviewed changes

eliserichards added the pr:needs-changes PRs that need some changes/fixes before they can land label Oct 16, 2020

Amejia481 reviewed Oct 20, 2020

View reviewed changes

...src/main/java/org/mozilla/fenix/library/downloads/viewholders/DownloadsListItemViewHolder.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Oct 20, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Oct 20, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt Outdated Show resolved Hide resolved

gabrielluong removed the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Oct 22, 2020

eliserichards reviewed Oct 27, 2020

View reviewed changes

kglazko force-pushed the PreserveViewDownloadsMenus branch from 90d7b71 to 8afdca3 Compare October 27, 2020 20:59

This was referenced Oct 27, 2020

Refactor library undo implementation to improve code quality #16215

Closed

Refactor library recycler adapters #16216

Closed

kglazko requested review from Amejia481 and eliserichards October 28, 2020 16:49

Amejia481 requested a review from boek October 28, 2020 21:11

eliserichards suggested changes Oct 29, 2020

View reviewed changes

boek reviewed Oct 30, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Oct 30, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Oct 30, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/library/downloads/DownloadFragment.kt Outdated Show resolved Hide resolved

kglazko force-pushed the PreserveViewDownloadsMenus branch 2 times, most recently from b8964bf to 13e0817 Compare November 2, 2020 18:41

kglazko requested review from eliserichards and boek November 2, 2020 18:41

Amejia481 self-requested a review November 3, 2020 21:45

Amejia481 approved these changes Nov 3, 2020

View reviewed changes

kglazko force-pushed the PreserveViewDownloadsMenus branch from 9df7da6 to 4d4beed Compare November 3, 2020 22:08

kglazko removed the request for review from eliserichards November 3, 2020 22:23

eliserichards approved these changes Nov 4, 2020

View reviewed changes

kglazko removed the pr:needs-changes PRs that need some changes/fixes before they can land label Nov 4, 2020

For mozilla-mobile#352: Delete a download

d9d1be5

kglazko force-pushed the PreserveViewDownloadsMenus branch from 4d4beed to d9d1be5 Compare November 4, 2020 19:35

kglazko merged commit 0ae2689 into mozilla-mobile:master Nov 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #352: Delete a download #15577

For #352: Delete a download #15577

kglazko commented Sep 30, 2020

eliserichards left a comment

kglazko commented Oct 15, 2020

codecov-io commented Oct 15, 2020 •

edited

eliserichards Oct 16, 2020

eliserichards Oct 27, 2020

Amejia481 Oct 29, 2020

Amejia481 commented Oct 20, 2020 •

edited

Amejia481 commented Oct 20, 2020

eliserichards left a comment

eliserichards left a comment

Amejia481 left a comment

eliserichards commented Nov 4, 2020

For #352: Delete a download #15577

For #352: Delete a download #15577

Conversation

kglazko commented Sep 30, 2020

Pull Request checklist

To download an APK when reviewing a PR:

eliserichards left a comment

Choose a reason for hiding this comment

kglazko commented Oct 15, 2020

codecov-io commented Oct 15, 2020 • edited

Codecov Report

eliserichards Oct 16, 2020

Choose a reason for hiding this comment

eliserichards Oct 27, 2020

Choose a reason for hiding this comment

Amejia481 Oct 29, 2020

Choose a reason for hiding this comment

Amejia481 commented Oct 20, 2020 • edited

Amejia481 commented Oct 20, 2020

eliserichards left a comment

Choose a reason for hiding this comment

eliserichards left a comment

Choose a reason for hiding this comment

Amejia481 left a comment

Choose a reason for hiding this comment

eliserichards commented Nov 4, 2020

codecov-io commented Oct 15, 2020 •

edited

Amejia481 commented Oct 20, 2020 •

edited