From 7d6b071d8a97757f282906397faf30656b86b486 Mon Sep 17 00:00:00 2001 From: relikd Date: Thu, 4 Jun 2020 17:07:37 +0200 Subject: [PATCH] Bugfixes - Disable cell animations for huge changes - Updating a cell keeps the old position whenever possible - Async `didChangeDateFilter` - Fixes bug where saving a recording would persist entries again - Small changes to `TimeFormat`, `AlertDeleteLogs` and `binTreeIndex()` --- main/Common Classes/FilterPipeline.swift | 60 ++++++++++++++----- .../Data Source/GroupedDomainDataSource.swift | 36 +++++------ main/Data Source/RecordingsDB.swift | 5 +- main/Data Source/SyncUpdate.swift | 35 +++++++---- main/Data Source/TestDataSource.swift | 3 +- main/Extensions/AlertSheet.swift | 24 +++----- main/Extensions/Array.swift | 18 ++++-- main/Extensions/Time.swift | 32 ++++++---- main/Recordings/VCEditRecording.swift | 7 ++- main/Recordings/VCRecordings.swift | 1 + main/Requests/TVCDomains.swift | 2 +- main/Requests/VCDateFilter.swift | 2 +- 12 files changed, 142 insertions(+), 83 deletions(-) diff --git a/main/Common Classes/FilterPipeline.swift b/main/Common Classes/FilterPipeline.swift index dbc256c..6f6d2e4 100644 --- a/main/Common Classes/FilterPipeline.swift +++ b/main/Common Classes/FilterPipeline.swift @@ -17,6 +17,8 @@ class FilterPipeline { private var display: PipelineSorting! private(set) weak var delegate: FilterPipelineDelegate? + private var cellAnimations: Bool = true + required init(withDelegate: FilterPipelineDelegate) { delegate = withDelegate } @@ -39,6 +41,8 @@ class FilterPipeline { /// - Returns: Index in `dataSource` and found object or `nil` if no matching item found. /// - Complexity: O(*n*), where *n* is the length of the `dataSource`. func dataSourceGet(where predicate: ((T) -> Bool)) -> (index: Int, object: T)? { + // TODO: use sorted dataSource for binary lookup? + // would require to shift filter and sorting indices for every new element guard let i = dataSource.firstIndex(where: predicate) else { return nil } @@ -166,6 +170,20 @@ class FilterPipeline { // MARK: data updates + /// Disable individual cell updates (update, move, insert & remove actions) + func pauseCellAnimations(if condition: Bool) { + cellAnimations = delegate?.tableView.isFrontmost ?? false && !condition + } + + /// Allow individual cell updates (update, move, insert & remove actions) if tableView `isFrontmost` + /// - Parameter reloadTable: If `true` and cell animations are disabled, perform `tableView.reloadData()` + func continueCellAnimations(reloadTable: Bool = false) { + if !cellAnimations { + cellAnimations = true + if reloadTable { delegate?.tableView.reloadData() } + } + } + /// Add new element to the original `dataSource` and immediately apply filter and sorting. /// - Complexity: O((*m*+1) log *n*), where *m* is the number of filters and *n* the number of elements in each filter. func addNew(_ obj: T) { @@ -176,7 +194,7 @@ class FilterPipeline { } // survived all filters let displayIndex = display.insertNew(index) - delegate?.tableView.safeInsertRow(displayIndex, with: .left) + if cellAnimations { delegate?.tableView.safeInsertRow(displayIndex, with: .left) } } /// Update element at `index` in the original `dataSource` and immediately re-apply filter and sorting. @@ -190,21 +208,22 @@ class FilterPipeline { let oldPos = display.deleteOld(index) dataSource[index] = obj guard status.display else { - if let old = oldPos { delegate?.tableView.safeDeleteRows([old]) } + if cellAnimations, oldPos != -1 { delegate?.tableView.safeDeleteRows([oldPos]) } return } - let newPos = display.insertNew(index) - if let old = oldPos { - if old == newPos { - delegate?.tableView.safeReloadRow(old) + let newPos = display.insertNew(index, previousIndex: oldPos) + guard cellAnimations else { return } + if oldPos == -1 { + delegate?.tableView.safeInsertRow(newPos, with: .left) + } else { + if oldPos == newPos { + delegate?.tableView.safeReloadRow(oldPos) } else { - delegate?.tableView.safeMoveRow(old, to: newPos) + delegate?.tableView.safeMoveRow(oldPos, to: newPos) if delegate?.tableView.isFrontmost ?? false { delegate?.rowNeedsUpdate(newPos) } } - } else { - delegate?.tableView.safeInsertRow(newPos, with: .left) } } @@ -221,7 +240,7 @@ class FilterPipeline { filter.shiftRemove(indices: sorted) } let indices = display.shiftRemove(indices: sorted) - delegate?.tableView.safeDeleteRows(indices) + if cellAnimations { delegate?.tableView.safeDeleteRows(indices) } } } @@ -368,20 +387,29 @@ class PipelineSorting { } /// Add new element and automatically sort according to predicate - /// - Parameter index: Index of the element position in the original `dataSource` + /// - Parameters: + /// - index: Index of the element position in the original `dataSource` + /// - prev: If greater than `0`, try re-insert at the same position. /// - Returns: Index in the projection /// - Complexity: O(log *n*), where *n* is the length of the `projection`. - @discardableResult fileprivate func insertNew(_ index: Int) -> Int { - projection.binTreeInsert(index, compare: comperator) + @discardableResult fileprivate func insertNew(_ index: Int, previousIndex prev: Int = -1) -> Int { + if prev >= 0, prev < projection.count { + if (prev == 0 || !comperator(index, projection[prev - 1])), !comperator(projection[prev], index) { + // If element can be inserted at the same position without resorting, do that + projection.insert(index, at: prev) + return prev + } + } + return projection.binTreeInsert(index, compare: comperator) } /// Remove element from projection /// - Parameter index: Index of the element position in the original `dataSource` - /// - Returns: Index in the projection or `nil` if element did not exist + /// - Returns: Index in the projection or `-1` if element did not exist /// - Complexity: O(*n*), where *n* is the length of the `projection`. - fileprivate func deleteOld(_ index: Int) -> Int? { + fileprivate func deleteOld(_ index: Int) -> Int { guard let i = projection.firstIndex(of: index) else { - return nil + return -1 } projection.remove(at: i) return i diff --git a/main/Data Source/GroupedDomainDataSource.swift b/main/Data Source/GroupedDomainDataSource.swift index b5383de..04e9cb8 100644 --- a/main/Data Source/GroupedDomainDataSource.swift +++ b/main/Data Source/GroupedDomainDataSource.swift @@ -52,10 +52,10 @@ class GroupedDomainDataSource { sync.pause() if let affectedDomain = notification?.object as? String { partiallyReloadFromSource(affectedDomain) - sync.start() + sync.continue() } else { pipeline.reload(fromSource: true, whenDone: { - sync.start() + sync.continue() refreshControl?.endRefreshing() }) } @@ -86,11 +86,14 @@ class GroupedDomainDataSource { /// Callback fired when background sync added new entries to the list. (`NotifySyncInsert` notification) @objc private func syncInsert(_ notification: Notification) { + sync.pause() + defer { sync.continue() } let range = notification.object as! SQLiteRowRange guard let latest = AppDB?.dnsLogsGrouped(range: range, parentDomain: parent) else { assertionFailure("NotifySyncInsert fired with empty range") return } + pipeline.pauseCellAnimations(if: latest.count > 14) for x in latest { if let (i, obj) = pipeline.dataSourceGet(where: { $0.domain == x.domain }) { pipeline.update(obj + x, at: i) @@ -101,16 +104,19 @@ class GroupedDomainDataSource { } tsLatest = max(tsLatest, x.lastModified) } + pipeline.continueCellAnimations(reloadTable: true) } /// Callback fired when background sync removed old entries from the list. (`NotifySyncRemove` notification) @objc private func syncRemove(_ notification: Notification) { + sync.pause() + defer { sync.continue() } let range = notification.object as! SQLiteRowRange guard let outdated = AppDB?.dnsLogsGrouped(range: range, parentDomain: parent), outdated.count > 0 else { - assertionFailure("NotifySyncRemove fired with empty range") return } + pipeline.pauseCellAnimations(if: outdated.count > 14) var listOfDeletes: [Int] = [] for x in outdated { guard let (i, obj) = pipeline.dataSourceGet(where: { $0.domain == x.domain }) else { @@ -124,6 +130,7 @@ class GroupedDomainDataSource { } } pipeline.remove(indices: listOfDeletes.sorted()) + pipeline.continueCellAnimations(reloadTable: true) } } @@ -144,11 +151,12 @@ extension GroupedDomainDataSource { return // nothing has changed } db.vacuum() - NotifyLogHistoryReset.postAsyncMain(domain) // calls deleteReloadFromSource(:) + NotifyLogHistoryReset.postAsyncMain(domain) // calls partiallyReloadFromSource(:) } } /// Reload a single data source entry. Callback fired by `reloadFromSource()` + /// Only useful if `affectedFQDN` currently exists in `dataSource`. Can either update or remove entry. private func partiallyReloadFromSource(_ affectedFQDN: String) { let affectedParent = affectedFQDN.extractDomain() guard parent == nil || parent == affectedParent else { @@ -159,20 +167,14 @@ extension GroupedDomainDataSource { // can only happen if delete sheet is open while background sync removed the element return } - var removeOld = true - if let new = AppDB?.dnsLogsGrouped(since: sync.tsEarliest, upto: tsLatest, matchingDomain: affected, parentDomain: parent) { - assert(new.count < 2) - for var x in new { - x.options = DomainFilter[x.domain] - if old.object.domain == x.domain { - pipeline.update(x, at: old.index) - removeOld = false - } else { - pipeline.addNew(x) - } - } + if var updated = AppDB?.dnsLogsGrouped(since: sync.tsEarliest, upto: tsLatest, + matchingDomain: affected, parentDomain: parent)?.first { + assert(old.object.domain == updated.domain) + updated.options = DomainFilter[updated.domain] + pipeline.update(updated, at: old.index) + } else { + pipeline.remove(indices: [old.index]) } - if removeOld { pipeline.remove(indices: [old.index]) } } } diff --git a/main/Data Source/RecordingsDB.swift b/main/Data Source/RecordingsDB.swift index 2246033..9d43f7c 100644 --- a/main/Data Source/RecordingsDB.swift +++ b/main/Data Source/RecordingsDB.swift @@ -14,7 +14,10 @@ enum RecordingsDB { static func list() -> [Recording] { AppDB?.recordingGetAll() ?? [] } /// Copy log entries from generic `heap` table to recording specific `recLog` table - static func persist(_ r: Recording) { AppDB?.recordingLogsPersist(r) } + static func persist(_ r: Recording) { + sync.syncNow() // persist changes in cache before copying recording details + AppDB?.recordingLogsPersist(r) + } /// Get list of domains that occured during the recording static func details(_ r: Recording) -> [RecordLog] { diff --git a/main/Data Source/SyncUpdate.swift b/main/Data Source/SyncUpdate.swift index ffbbf21..21b4792 100644 --- a/main/Data Source/SyncUpdate.swift +++ b/main/Data Source/SyncUpdate.swift @@ -11,34 +11,45 @@ class SyncUpdate { timer = Timer.repeating(interval, call: #selector(periodicUpdate), on: self) } + @objc private func periodicUpdate() { if paused == 0 { syncNow() } } + @objc private func didChangeDateFilter() { let lastXFilter = Pref.DateFilter.lastXMinTimestamp() ?? 0 - if tsEarliest < lastXFilter { - if let excess = AppDB?.dnsLogsRowRange(between: tsEarliest, and: lastXFilter) { - NotifySyncRemove.post(excess) + let before = tsEarliest + tsEarliest = lastXFilter + if before < lastXFilter { + DispatchQueue.global().async { + if let excess = AppDB?.dnsLogsRowRange(between: before, and: lastXFilter) { + NotifySyncRemove.postAsyncMain(excess) + } } - } else if tsEarliest > lastXFilter { - if let missing = AppDB?.dnsLogsRowRange(between: lastXFilter, and: tsEarliest) { - NotifySyncInsert.post(missing) + } else if before > lastXFilter { + DispatchQueue.global().async { + if let missing = AppDB?.dnsLogsRowRange(between: lastXFilter, and: before) { + NotifySyncInsert.postAsyncMain(missing) + } } } - tsEarliest = lastXFilter } + func start() { paused = 0 } func pause() { paused += 1 } - func start() { if paused > 0 { paused -= 1 } } + func `continue`() { if paused > 0 { paused -= 1 } } - @objc private func periodicUpdate() { - guard paused == 0, let db = AppDB else { return } - if let inserted = db.dnsLogsPersist() { // move cache -> heap + func syncNow() { + self.pause() // reduce concurrent load + + if let inserted = AppDB?.dnsLogsPersist() { // move cache -> heap NotifySyncInsert.post(inserted) } if let lastXFilter = Pref.DateFilter.lastXMinTimestamp(), tsEarliest < lastXFilter { - if let removed = db.dnsLogsRowRange(between: tsEarliest, and: lastXFilter) { + if let removed = AppDB?.dnsLogsRowRange(between: tsEarliest, and: lastXFilter) { NotifySyncRemove.post(removed) } tsEarliest = lastXFilter } // TODO: periodic hard delete old logs (will reset rowids!) + + self.continue() } } diff --git a/main/Data Source/TestDataSource.swift b/main/Data Source/TestDataSource.swift index cb0970b..bcfd84f 100644 --- a/main/Data Source/TestDataSource.swift +++ b/main/Data Source/TestDataSource.swift @@ -11,7 +11,8 @@ class TestDataSource { QLog.Debug("SQLite path: \(URL.internalDB())") let deleted = db.dnsLogsDelete("test.com", strict: false) - QLog.Debug("Deleting \(deleted) rows matching 'test.com'") + try? db.run(sql: "DELETE FROM cache;") + QLog.Debug("Deleting \(deleted) rows matching 'test.com' (+ \(db.numberOfChanges) in cache)") QLog.Debug("Writing 33 test logs") pStmt = try! db.logWritePrepare() diff --git a/main/Extensions/AlertSheet.swift b/main/Extensions/AlertSheet.swift index fd5c300..8cd4af6 100644 --- a/main/Extensions/AlertSheet.swift +++ b/main/Extensions/AlertSheet.swift @@ -43,7 +43,7 @@ func AskAlert(title: String?, text: String?, buttonText: String = "Continue", bu /// - buttons: Default: `[]` /// - lastIsDestructive: Default: `false` /// - cancelButtonText: Default: `"Dismiss"` -func BottomAlert(title: String?, text: String?, buttons: [String] = [], lastIsDestructive: Bool = false, cancelButtonText: String = "Dismiss", callback: @escaping (_ index: Int?) -> Void) -> UIAlertController { +func BottomAlert(title: String?, text: String?, buttons: [String] = [], lastIsDestructive: Bool = false, cancelButtonText: String = "Cancel", callback: @escaping (_ index: Int?) -> Void) -> UIAlertController { let alert = UIAlertController(title: title, message: text, preferredStyle: .actionSheet) for (i, btn) in buttons.enumerated() { let dangerous = (lastIsDestructive && i + 1 == buttons.count) @@ -54,21 +54,13 @@ func BottomAlert(title: String?, text: String?, buttons: [String] = [], lastIsDe } func AlertDeleteLogs(_ domain: String, latest: Timestamp, success: @escaping (_ tsMin: Timestamp) -> Void) -> UIAlertController { - let sinceNow = Timestamp.now() - latest - var buttons = ["Last 5 minutes", "Last 15 minutes", "Last hour", "Last 24 hours", "Delete everything"] - var times: [Timestamp] = [300, 900, 3600, 86400] - while times.count > 0, times[0] < sinceNow { - buttons.removeFirst() - times.removeFirst() - } - return BottomAlert(title: "Delete logs", text: "Delete logs for domain '\(domain)'", buttons: buttons, lastIsDestructive: true, cancelButtonText: "Cancel") { - guard let idx = $0 else { - return - } - if idx >= times.count { - success(0) - } else { - success(Timestamp.now() - times[idx]) + let minutesPassed = (Timestamp.now() - latest) / 60 + let times: [Int] = [5, 15, 60, 1440].compactMap { minutesPassed < $0 ? $0 : nil } + let fmt = TimeFormat(.full, allowed: [.hour, .minute]) + let labels = times.map { "Last " + (fmt.from(minutes: $0) ?? "?") } + return BottomAlert(title: "Delete logs", text: "Delete logs for domain '\(domain)'", buttons: labels + ["Delete everything"], lastIsDestructive: true) { + if let i = $0 { + success(i < times.count ? Timestamp.past(minutes: times[i]) : 0) } } } diff --git a/main/Extensions/Array.swift b/main/Extensions/Array.swift index cc87678..2ef1c7d 100644 --- a/main/Extensions/Array.swift +++ b/main/Extensions/Array.swift @@ -19,10 +19,13 @@ extension Array { /// Binary tree search operation. /// - Warning: Array must be sorted already. - /// - Parameter mustExist: Determine whether to return low index or `nil` if element is missing. + /// - Parameters: + /// - mustExist: Determine whether to return low index or `nil` if element is missing. + /// - first: If `true`, keep searching for first matching element. /// - Returns: Index or `nil` (only if `mustExist = true` and element does not exist). /// - Complexity: O(log *n*), where *n* is the length of the array. - func binTreeIndex(of element: Element, compare fn: CompareFn, mustExist: Bool = false) -> Int? { + func binTreeIndex(of element: Element, compare fn: CompareFn, mustExist: Bool = false, findFirst: Bool = false) -> Int? { + var found = false var lo = 0, hi = self.count - 1 while lo <= hi { let mid = (lo + hi)/2 @@ -31,10 +34,17 @@ extension Array { } else if fn(element, self[mid]) { hi = mid - 1 } else { - return mid + if !findFirst { return mid } // exit early if we dont care about first index + hi = mid - 1 + found = true } } - return mustExist ? nil : lo // not found, would be inserted at position lo + return (mustExist && !found) ? nil : lo // not found, would be inserted at position lo + } + + /// Binary tree lookup whether element exists. Performs `binTreeIndex(of:compare:mustExist:)` internally. + func binTreeExists(_ element: Element, compare fn: CompareFn) -> Bool { + binTreeIndex(of: element, compare: fn, mustExist: true) != nil } /// Binary tree insert operation diff --git a/main/Extensions/Time.swift b/main/Extensions/Time.swift index d613167..a740951 100644 --- a/main/Extensions/Time.swift +++ b/main/Extensions/Time.swift @@ -39,7 +39,27 @@ extension Timer { } } +// MARK: - TimeFormat + struct TimeFormat { + private var formatter: DateComponentsFormatter + + /// Init new formatter with exactly 1 unit count. E.g., `61 min -> 1 hr` + /// - Parameter allowed: Default: `[.day, .hour, .minute, .second]` + init(_ style: DateComponentsFormatter.UnitsStyle, allowed: NSCalendar.Unit = [.day, .hour, .minute, .second]) { + formatter = DateComponentsFormatter() + formatter.maximumUnitCount = 1 + formatter.allowedUnits = allowed + formatter.unitsStyle = style + } + + /// Formatted duration string, e.g., `20 min` or `7 days` + func from(days: Int = 0, hours: Int = 0, minutes: Int = 0, seconds: Int = 0) -> String? { + formatter.string(from: DateComponents(day: days, hour: hours, minute: minutes, second: seconds)) + } + + // MARK: static + /// Time string with format `HH:mm` static func from(_ duration: Timestamp) -> String { String(format: "%02d:%02d", duration / 60, duration % 60) @@ -59,16 +79,4 @@ struct TimeFormat { static func since(_ date: Date, millis: Bool = false) -> String { from(Date().timeIntervalSince(date), millis: millis) } - - /// Formatted duration string, e.g., `20 min` or `7 days` - /// - Parameters: - /// - minutes: Duration in minutes - /// - style: Default: `.short` - static func short(minutes: Int, style: DateComponentsFormatter.UnitsStyle = .short) -> String? { - let dcf = DateComponentsFormatter() - dcf.maximumUnitCount = 1 - dcf.allowedUnits = [.day, .hour, .minute] - dcf.unitsStyle = style - return dcf.string(from: DateComponents(minute: minutes)) - } } diff --git a/main/Recordings/VCEditRecording.swift b/main/Recordings/VCEditRecording.swift index 74dee80..58c18f9 100644 --- a/main/Recordings/VCEditRecording.swift +++ b/main/Recordings/VCEditRecording.swift @@ -35,7 +35,8 @@ class VCEditRecording: UIViewController, UITextFieldDelegate, UITextViewDelegate // MARK: Save & Cancel Buttons @IBAction func didTapSave(_ sender: UIBarButtonItem) { - if deleteOnCancel { // aka newly created + let newlyCreated = deleteOnCancel + if newlyCreated { // if remains true, `viewDidDisappear` will delete the record deleteOnCancel = false } @@ -44,7 +45,9 @@ class VCEditRecording: UIViewController, UITextFieldDelegate, UITextViewDelegate record.notes = (inputNotes.text == "") ? nil : inputNotes.text dismiss(animated: true) { RecordingsDB.update(self.record) - RecordingsDB.persist(self.record) + if newlyCreated { + RecordingsDB.persist(self.record) + } } } diff --git a/main/Recordings/VCRecordings.swift b/main/Recordings/VCRecordings.swift index ee0c540..b789d6a 100644 --- a/main/Recordings/VCRecordings.swift +++ b/main/Recordings/VCRecordings.swift @@ -38,6 +38,7 @@ class VCRecordings: UIViewController, UINavigationControllerDelegate { } func navigationController(_ nav: UINavigationController, didShow vc: UIViewController, animated: Bool) { + // TODO: use interactive animation handler to dynamically animate "new recording" view hideNewRecording(isRootVC: (vc == nav.viewControllers.first), didShow: true) } diff --git a/main/Requests/TVCDomains.swift b/main/Requests/TVCDomains.swift index c7f6be2..9965b5f 100644 --- a/main/Requests/TVCDomains.swift +++ b/main/Requests/TVCDomains.swift @@ -117,7 +117,7 @@ class TVCDomains: UITableViewController, UISearchBarDelegate, FilterPipelineDele case .LastXMin: // most recent let lastXMin = Pref.DateFilter.LastXMin if lastXMin == 0 { fallthrough } - self.filterButtonDetail.title = TimeFormat.short(minutes: lastXMin, style: .abbreviated) + self.filterButtonDetail.title = TimeFormat(.abbreviated).from(minutes: lastXMin) self.filterButton.image = UIImage(named: "filter-filled") default: self.filterButtonDetail.title = "" diff --git a/main/Requests/VCDateFilter.swift b/main/Requests/VCDateFilter.swift index f715020..3fa200b 100644 --- a/main/Requests/VCDateFilter.swift +++ b/main/Requests/VCDateFilter.swift @@ -54,7 +54,7 @@ class VCDateFilter: UIViewController, UIGestureRecognizerDelegate { sender.value = Float(i) / 9 if sender.tag != durationTimes[i] { sender.tag = durationTimes[i] - durationLabel.text = (sender.tag == 0 ? "Off" : TimeFormat.short(minutes: sender.tag)) + durationLabel.text = (sender.tag == 0 ? "Off" : TimeFormat(.short).from(minutes: sender.tag)) } }