Clear storage #376

mneuwert · 2019-05-08T18:08:55Z

Description

Added option in settings to clean up offline file storage. Also showing how much space is occupied by downloaded files and how much space is available on the device.

Related Issue

#260

Motivation and Context

Helps to avoid cluttering user's device with downloaded offline data.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

Test Plan:

https://github.com/owncloud/QA/blob/master/Mobile/iOS-app/Clear%20Storage.md

CLAassistant · 2019-05-08T18:09:04Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ mneuwert
✅ hosy
❌ felix-schwarz
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

felix-schwarz

Already looking good, just a few points:

I assumed the plan was to create an additional "Info" view (with just "Certificates" + "Free space/Compacting" for now) instead of adding this to the BookmarkViewController. By adding it there, I have to choose between "Cancel" and "Save" to return to the server list, which could be confusing to some users.
please update the ios-sdk to the latest develop commit, so the Files app running side by side will get notified of updates, too

ownCloud/Bookmarks/BookmarkViewController.swift

ownCloud/Tools/FileManager+Extension.swift

mneuwert · 2019-05-09T13:15:23Z

Already looking good, just a few points:

I assumed the plan was to create an additional "Info" view (with just "Certificates" + "Free space/Compacting" for now) instead of adding this to the BookmarkViewController. By adding it there, I have to choose between "Cancel" and "Save" to return to the server list, which could be confusing to some users.

please update the ios-sdk to the latest develop commit, so the Files app running side by side will get notified of updates, too

@felix-schwarz In our last meeting I suggested to put the storage information inside the "Edit" view to avoid adding third swipe action in the server list. And.. it felt just right to do so, since all this stuff is related. So, we agreed ion that approach. I will fix the findings you mentioned.

mneuwert · 2019-05-09T13:21:34Z

@felix-schwarz updated SDK in the branch to develop/LATEST

felix-schwarz

Code-wise everything's fine, so I'm approving this PR. @mneuwert

@felix-schwarz In our last meeting I suggested to put the storage information inside the "Edit" view to avoid adding third swipe action in the server list. And.. it felt just right to do so, since all this stuff is related. So, we agreed ion that approach. I will fix the findings you mentioned.

I'd certainly agree at first, too, but seeing it in practice, I'm not as sure:

"Edit" implies that I'm editing the connection. I wouldn't expect cleanup options here.
on iPhone 6-8, the option "Delete offline copies" is not visible until I scroll. On an iPhone SE, I have to scroll before seeing the Storage section at all
"Delete Offline Copies" is not a read-only option. Having to choose between "Cancel" and "Save" to return to the list could imply to users that offline copies are not actually deleted if "Cancel" is tapped afterwards (just like anything else in that edit UI will not take effect if "Cancel is tapped) - or that "Save" needs to be tapped for the deletion to actually take place

Compared to that a separate "Info" or "Manage" option would have clearer labelling, show the options without scrolling and just have a "Close", "Back" or "Done" button at the top. It would also be reusable in a branding context where BookmarkViewController is not used.

iPhone 6-8	iPhone SE

codecov · 2019-05-09T21:54:01Z

Codecov Report

Merging #376 into master will decrease coverage by 0.14%.
The diff coverage is 32.89%.

@@            Coverage Diff            @@
##           master    #376      +/-   ##
=========================================
- Coverage   30.35%   30.2%   -0.15%     
=========================================
  Files         241     243       +2     
  Lines       16790   16894     +104     
=========================================
+ Hits         5096    5103       +7     
- Misses      11694   11791      +97

Impacted Files	Coverage Δ
...wnCloud/Bookmarks/BookmarkInfoViewController.swift	`0% <0%> (ø)`
ownCloud/Tools/FileManager+Extension.swift	`0% <0%> (ø)`
...nCloud/Settings/UserInterfaceSettingsSection.swift	`87.03% <100%> (ø)`	⬆️
ownCloud/UI Elements/StaticTableViewRow.swift	`90.9% <60%> (-0.61%)`	⬇️
...ud/Server List/ServerListTableViewController.swift	`70.74% <73.01%> (-1.9%)`	⬇️
ownCloud/Client/ClientRootViewController.swift	`68.42% <0%> (-0.41%)`	⬇️

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 050fd96...065ade3. Read the comment docs.

mneuwert · 2019-05-09T22:27:54Z

Compared to that a separate "Info" or "Manage" option would have clearer labelling, show the options without scrolling and just have a "Close", "Back" or "Done" button at the top. It would also be reusable in a branding context where BookmarkViewController is not used.

@felix-schwarz Addressed this.. I agree that especially given the context of non-destructive changes which are cancellable, storage management options are better placed in a different dedicated view.

hosy · 2019-05-11T18:35:50Z

@mneuwert looks cool! I would suggest to change the background Color for the row action to make a difference to the edit action.

mneuwert · 2019-05-13T13:23:02Z

@mneuwert looks cool! I would suggest to change the background Color for the row action to make a difference to the edit action.

Yes, absolutely, changed in latest commit however was not sure if the color should be defined in the color scheme. Also not sure if this blue is not "too blue":) But this is rather question to our UX guy.

michaelstingl · 2019-05-13T15:17:58Z

Thanks for posting the screenshot. Not sure if I like so many items in the swipe buttons? Make "Manage" a sub-view from from "Edit"? (Like "Advanced" sub-view in the system Settings)

mneuwert · 2019-05-13T15:27:22Z

@michaelstingl I actually think (as already pointed out by @felix-schwarz) it is not a good idea to put storage management under "Edit account" since this screen has options to cancel and save changes, but "delete offline files" is rather an action which is performed immediately and will not e.g. be undone if user taps "cancel". May be we could restructure "Edit account" UI in some way which which would allow us to separate these concerns.

michaelstingl · 2019-05-13T15:45:08Z

Yeah, open for ideas…

# Conflicts: # ios-sdk # ownCloud.xcodeproj/project.pbxproj # ownCloud/Resources/en.lproj/Localizable.strings

jesmrec · 2019-05-15T12:32:59Z

Sorry for the confusion with labels, i thought i got a weird behaviour but it was my fault.

Approved! great job!

mneuwert and others added 3 commits May 8, 2019 13:45

Added storage section

7e6cb02

- Adding "Compact" row action to server list

15e675f

First implementation of clear storage feature

b5afa42

mneuwert requested review from felix-schwarz and jesmrec May 8, 2019 18:08

Fixed swiftLint finding

5d773cd

jesmrec assigned mneuwert and jesmrec May 9, 2019

felix-schwarz requested changes May 9, 2019

View reviewed changes

ownCloud/Bookmarks/BookmarkViewController.swift Outdated Show resolved Hide resolved

ownCloud/Tools/FileManager+Extension.swift Outdated Show resolved Hide resolved

Fixed review findings

5ffab22

felix-schwarz approved these changes May 9, 2019

View reviewed changes

Moved offline storage management functionality into new view controllerr

63044df

Changed background color of row action

a57c876

Merge branch 'master' into feature/clear_storage

266973b

# Conflicts: # ios-sdk # ownCloud.xcodeproj/project.pbxproj # ownCloud/Resources/en.lproj/Localizable.strings

jesmrec approved these changes May 15, 2019

View reviewed changes

jesmrec added the Approved by QA Approved by QA label May 15, 2019

jesmrec mentioned this pull request May 15, 2019

Swipe options in bookmark list are misplaced under orientation changes #381

Open

jesmrec removed the Approved by QA Approved by QA label May 15, 2019

jesmrec added the Approved by QA Approved by QA label May 15, 2019

Merge branch 'master' into feature/clear_storage

065ade3

hosy merged commit 3a5c4e8 into master May 15, 2019

delete-merged-branch bot deleted the feature/clear_storage branch May 15, 2019 16:56

jesmrec mentioned this pull request Jun 14, 2019

Roadmap #94

Open

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear storage #376

Clear storage #376

mneuwert commented May 8, 2019 •

edited by jesmrec

Loading

CLAassistant commented May 8, 2019 •

edited

Loading

felix-schwarz left a comment •

edited

Loading

mneuwert commented May 9, 2019 •

edited

Loading

mneuwert commented May 9, 2019

felix-schwarz left a comment •

edited

Loading

codecov bot commented May 9, 2019 •

edited

Loading

mneuwert commented May 9, 2019

hosy commented May 11, 2019

mneuwert commented May 13, 2019 •

edited

Loading

michaelstingl commented May 13, 2019 •

edited

Loading

mneuwert commented May 13, 2019 •

edited

Loading

michaelstingl commented May 13, 2019

jesmrec commented May 15, 2019 •

edited

Loading

Clear storage #376

Clear storage #376

Conversation

mneuwert commented May 8, 2019 • edited by jesmrec Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Test Plan:

CLAassistant commented May 8, 2019 • edited Loading

felix-schwarz left a comment • edited Loading

Choose a reason for hiding this comment

mneuwert commented May 9, 2019 • edited Loading

mneuwert commented May 9, 2019

felix-schwarz left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented May 9, 2019 • edited Loading

Codecov Report

mneuwert commented May 9, 2019

hosy commented May 11, 2019

mneuwert commented May 13, 2019 • edited Loading

michaelstingl commented May 13, 2019 • edited Loading

mneuwert commented May 13, 2019 • edited Loading

michaelstingl commented May 13, 2019

jesmrec commented May 15, 2019 • edited Loading

mneuwert commented May 8, 2019 •

edited by jesmrec

Loading

CLAassistant commented May 8, 2019 •

edited

Loading

felix-schwarz left a comment •

edited

Loading

mneuwert commented May 9, 2019 •

edited

Loading

felix-schwarz left a comment •

edited

Loading

codecov bot commented May 9, 2019 •

edited

Loading

mneuwert commented May 13, 2019 •

edited

Loading

michaelstingl commented May 13, 2019 •

edited

Loading

mneuwert commented May 13, 2019 •

edited

Loading

jesmrec commented May 15, 2019 •

edited

Loading