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.

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

[feature/se-concurrency] Sync Engine concurrency support #369

Merged
merged 94 commits into from
May 13, 2019

Conversation

felix-schwarz
Copy link
Contributor

@felix-schwarz felix-schwarz commented Apr 30, 2019

Description

This PR adds Sync Engine concurrency support to the app, so that more than one Sync Action (like move, copy, delete, download, upload, …) can be executed at once. To enable this, the Sync Engine:

  • builds a tree of dependencies between the scheduled actions by putting them in Sync Lanes
  • performs actions without dependencies on other actions immediately
  • queues actions that depend on the completion of preceding actions and performs them when it is their turn

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)

Test plan:

https://github.com/owncloud/QA/blob/master/Mobile/iOS-app/Concurrency.md

Bugs & improvements:

jesmrec and others added 30 commits April 4, 2019 10:17
- Fix separator insets in item list
- fixed long status messages, while setting dynamically numbers of lines
…rk and whenever editing a bookmark and saving it (previously UI seemed stuck for a couple of seconds)
…tion for OCErrors so they are included when serialized, so they become visible in the Files app, too

- Fixing issue (8) in #237
- Remove trash method and permission flag for items in FP, so "Delete Now" is the only option and Files app shows a warning
- Fixing (11) in #237
- Add new action extension location .plusButton
- Add Action extensions support to provide UIAlertAction objects
- Add .plusButton location to CreateFolder action
- Extend the ClientQueryViewController plusBarButton to include action extensions in the alert that's being shown
- Implementing improvement (15) from #237
… distinct Action Extensions

- Extend ActionProgressHandler to cater for both publishing and unpublishing progress objects
- Replace repeated code with a method to provide a ActionProgressHandler in ClientItemQueryViewController
- Make CreateFolderAction require a folder as parent item
- Make MoveAction present on the provided ViewController, not its navigationController
- Extend error handling in several places
	- remove create folder button from left
	- left-align the sort option and rename it "Sort by" (from "Sorted by")
	- turn the sort option button from a ThemeButton to a UIButton and replace the unicode arrow with an elegant chevron
	- remove sortBar location for action extensions
	- clean up SortBar code
- NSObject+ThemeApplication
	- replace use of isKindOfClass and force-casts with if let constructs
	- add support for UIButton themeing
- fix "flashing" reording right after opening a connection by setting the OCQuery sort method before starting the query
- fix bug where an alert was shown for invalid credentials, but the Edit button did not work
	- fix ClientRootViewController.closeClient(), so it calls the completionHandler
	- adjust the editing method to the changed view and navigation controller structure
- minor code formatting and style fixes in several places
- Fix spelling of CreateFolderAction.identifier value
- Simplify implementation of DuplicateAction by refactoring it to use new SDK APIs, addressing issue (7) in #237
- Fix potential future crash in ClientDirectoryPickerViewController by avoiding force-casting
- Make OCItem.parentItem in OCItem+Extension more robust using new SDK API
- Adapt FileProviderExtension to use new, supported SDK APIs instead of the island implementation that was -findKnownExistingItemInParent:withName:
- Add (and comment out) code that lets FileProviderExtension.createDirectoryWithName return as soon as the placeholder item is available
- Add empty implementations for trashItemWithIdentifier/untrashItemWithIdentifier/setLastUsedDate/supportedServiceSourcesForItemIdentifier to comply with requirement of overriding all methods raised in the documentation
mneuwert and others added 3 commits May 3, 2019 16:31
	- now groups progress by type and provides summaries such as "Uploading X of Y files.." that are easier to follow for the user
	- now prioritizes newer progress submissions over old ones, taking into account the possbility these may be executed earlier due to Sync Engine concurrency
@felix-schwarz
Copy link
Contributor Author

felix-schwarz commented May 6, 2019

(1) should be addressed by d744a44 with the following changes:

  • progress objects of the same type are now automatically grouped and summarized like this:

PNG-Bild 4

  • more recent progress objects (and groups) are now prioritized over older ones (it was the other way round before), so that if you're uploading 100 files, it initially shows the summary, but will switch to the progress info of whatever you decided to do after that for as long as that other action is ongoing.

  • looks like the code submitting the progress of downloads was removed at some point. Looking at adding the missing lines back in now.

	- Remove unused original DisplayHostViewController
	- Rename GalleryHostViewController to DisplayViewController
	- move Viewer code below Client where it logically belongs and split it up into subdirectories
- (Re-)add download progress reporting to ClientQueryViewController and initially add it to Display*ViewController classes
@felix-schwarz
Copy link
Contributor Author

Re (1) again: re-added the missing lines, so download progress is now once again shown in the progress summary bar

@jesmrec jesmrec added the Estimation - 5 (L) 5 points label May 7, 2019
@jesmrec
Copy link
Contributor

jesmrec commented May 8, 2019

(2)

With the last update related the behaviour of the progress bar, i notice this:

Using the default budget:

OCSyncActionCategoryTransfer : @(6),	// Limit total number of concurrent transfers to 6
OCSyncActionCategoryUpload   : @(3),	// Limit number of concurrent upload transfers to 3
OCSyncActionCategoryDownload : @(3)	// Limit number of concurrent download transfers to 3
  1. Tap on seven files to download
  2. Three of them start to download, remaining four enqueued -> Downloading 7 of 7 files
  3. Two of them finish -> Downloading 5 of 7 files
  4. From the file list add three extra files to download

Current: Downloading 8 of 10 files. But in the list, there are 8 files instead of 10. When everything is finished, new transfers trigger a new countdown.

Suggested (more ideas welcome):

if the progress message is Downloading X of Y files:

X -> ongoing transfers (maximum 6 for the default budget, or separate for uploads/downloads)
Y -> pending/non-finished transfers (number of cells in status view)

so, it counts only the pending stuff because the finished one is out of scope in terms of Status.

what do you think? CC @michaelstingl

@felix-schwarz
Copy link
Contributor Author

felix-schwarz commented May 9, 2019

Thanks for the feedback @jesmrec!

if the progress message is Downloading X of Y files:

X -> ongoing transfers (maximum 6 for the default budget, or separate for uploads/downloads)
Y -> pending/non-finished transfers (number of cells in status view)

so, it counts only the pending stuff because the finished one is out of scope in terms of Status.

I think that would be confusing to the average user and not provide much additional information. Most of the time it'd be Downloading 3 of Y files, maybe with brief fluctuations to Downloading 2 of Y files or Downloading 1 of Y files in-between as previous downloads finish and it takes a moment for new ones to be started.

There's also a mathematical problem with dropping the finished actions from the count. Imagine 5 transfers, with one at 90%, the rest at 0%. That'd result in an overall progress of 18% ((0.9 + 0 + 0 + 0 + 0)/5 = 0.18).

If all downloads then progress by 10%, the one with 90% hits 100% and is removed. The previously evenly advancing overall progress then suddenly drops from 18% to 10% ((0.1 + 0.1 + 0.1 + 0.1)/4 = 0.1), so the progress bar would jump around, which a progress bar should ideally never do.

That's why actions keep getting added to the count until the count is back to 1, at which point the progress of the last download can be shown directly.

I originally started out with messages following the pattern Uploading X files…, which is what f.ex. the macOS Finder does. It only shows the number of remaining files.

I then changed it to the current Uploading X of Y files… part out of curiosity how it'd feel, part because it helped me debug that feature. At this point, I'm still not fully convinced that change was/is a real improvement.

So with you bringing it up, I'd be totally fine to change it back to Uploading [number of remaining uploads] files… ("Finder style").

@jesmrec
Copy link
Contributor

jesmrec commented May 9, 2019

Ok, i got your idea and make sense to do it in that way. Let's give a try.

@jesmrec
Copy link
Contributor

jesmrec commented May 10, 2019

Approved on my side, correct application of concurrency taking in account the budgets.

@jesmrec jesmrec added the Approved by QA Approved by QA label May 10, 2019
…ded in the test target) so tests pass

- Remove XCTestsCase+Extension as it provides no benefit over Xcode's own messages whn tests start and finish
@michaelstingl michaelstingl merged commit a9f997e into master May 13, 2019
@delete-merged-branch delete-merged-branch bot deleted the feature/se-concurrency branch May 13, 2019 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants