Skip to content

Commit

Permalink
Fix datasource updates (#1040)
Browse files Browse the repository at this point in the history 
* Disallow annotation deletion for other authors

* Confine ExpandableCollectionsCollectionViewHandler datasource updates

Confine ExpandableCollectionsCollectionViewHandler datasource updates
to the update queue thread

* Confine PDFAnnotationsViewController datasource updates

Confine PDFAnnotationsViewController datasource updates to the update
queue thread
Do not setup observing when an AnnotationCell is reconfigured

* Improve PDFAnnotationsViewController UI updates

* Improve PDFAnnotationsViewController reloadHeight action

* Improve PDFAnnotationsViewController UI update

* Improve PDFAnnotationsViewController code

* Refactor PDFAnnotationsViewController
  • Loading branch information
@mvasilak
mvasilak authored Dec 11, 2024
1 parent 77c5525 commit 583a83c
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 104 deletions.
2 changes: 1 addition & 1 deletion Zotero/Scenes/Detail/PDF/Models/PDFDatabaseAnnotation.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ struct PDFDatabaseAnnotation {
if !library.metadataEditable {
return .notEditable
}
return isAuthor(currentUserId: currentUserId) ? .editable : .deletable
return isAuthor(currentUserId: currentUserId) ? .editable : .notEditable
}
}

Expand Down
3 changes: 1 addition & 2 deletions Zotero/Scenes/Detail/PDF/Views/Annotation View/AnnotationView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ final class AnnotationView: UIView {
)
setup(comment: comment, canEdit: canEdit)
setup(tags: annotation.tags, canEdit: canEdit, accessibilityEnabled: selected)
setupObserving()

let commentButtonIsHidden = commentTextView.isHidden
let highlightContentIsHidden = highlightContent?.isHidden ?? true
Expand Down Expand Up @@ -283,7 +282,7 @@ final class AnnotationView: UIView {
header.menuTap.flatMap({ Observable.just(Action.options($0)) }).bind(to: actionPublisher)
}

private func setupObserving() {
func setupObserving() {
var disposables: [Disposable] = buildDisposables()
if let doneTap = header.doneTap {
disposables.append(doneTap.flatMap({ Observable.just(Action.done) }).bind(to: actionPublisher))
Expand Down
5 changes: 5 additions & 0 deletions Zotero/Scenes/Detail/PDF/Views/AnnotationCell.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ final class AnnotationCell: UITableViewCell {
annotationView.resignFirstResponder()
}

let reconfiguringForSameAnnotation = key == annotation.key
key = annotation.key
selectionView.layer.borderWidth = selected ? PDFReaderLayout.cellSelectionLineWidth : 0
let availableWidth = availableWidth - (PDFReaderLayout.annotationLayout.horizontalInset * 2)
Expand All @@ -121,6 +122,10 @@ final class AnnotationCell: UITableViewCell {
pdfAnnotationsCoordinatorDelegate: pdfAnnotationsCoordinatorDelegate,
state: state
)
if !reconfiguringForSameAnnotation {
annotationView.setupObserving()
}
// Otherwise, reconfigured cells do not have their prepareForReuse method called, so observing is already set up.

setupAccessibility(
isAuthor: annotation.isAuthor(currentUserId: currentUserId),
Expand Down
186 changes: 95 additions & 91 deletions Zotero/Scenes/Detail/PDF/Views/PDFAnnotationsViewController.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,15 +69,9 @@ final class PDFAnnotationsViewController: UIViewController {
super.viewIsAppearing(animated)

tableView.setEditing(viewModel.state.sidebarEditingEnabled, animated: false)
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = NSDiffableDataSourceSnapshot<Int, PDFReaderState.AnnotationKey>()
snapshot.appendSections([0])
snapshot.appendItems(viewModel.state.sortedKeys)
dataSource.apply(snapshot, animatingDifferences: false) { [weak self] in
guard let self, let key = viewModel.state.focusSidebarKey, let indexPath = dataSource.indexPath(for: key) else { return }
tableView.selectRow(at: indexPath, animated: false, scrollPosition: .middle)
}
updateUI(state: viewModel.state, animatedDifferences: false) { [weak self] in
guard let self, let key = viewModel.state.focusSidebarKey, let indexPath = dataSource.indexPath(for: key) else { return }
tableView.selectRow(at: indexPath, animated: false, scrollPosition: .middle)
}

viewModel.stateObservable
Expand Down Expand Up @@ -143,14 +137,38 @@ final class PDFAnnotationsViewController: UIViewController {
viewModel.process(action: .setComment(key: annotation.key, comment: comment))

case .reloadHeight:
updateCellHeight()
focusSelectedCell()
reconfigureSelectedCellIfAny()

case .setCommentActive(let isActive):
viewModel.process(action: .setCommentActive(isActive))

case .done: break // Done button doesn't appear here
}

func reconfigureSelectedCellIfAny() {
guard let key = viewModel.state.selectedAnnotationKey else { return }
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = dataSource.snapshot()
snapshot.reconfigureItems([key])
dataSource.apply(snapshot, animatingDifferences: false) { [weak self] in
self?.focusSelectedCell()
}
}
}
}

private func updateUI(state: PDFReaderState, animatedDifferences: Bool = true, completion: (() -> Void)? = nil) {
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = NSDiffableDataSourceSnapshot<Int, PDFReaderState.AnnotationKey>()
snapshot.appendSections([0])
snapshot.appendItems(state.sortedKeys)
if let keys = state.updatedAnnotationKeys {
snapshot.reconfigureItems(keys)
}
dataSource.apply(snapshot, animatingDifferences: animatedDifferences, completion: completion)
}
}

private func update(state: PDFReaderState) {
Expand All @@ -160,117 +178,99 @@ final class PDFAnnotationsViewController: UIViewController {
emptyLabel.isHidden = !tableView.isHidden
}

reloadIfNeeded(for: state) { [weak self] in
let isVisible = parentDelegate?.isSidebarVisible ?? false
reloadIfNeeded(for: state, isVisible: isVisible) { [weak self] in
guard let self else { return }

if let keys = state.loadedPreviewImageAnnotationKeys {
updatePreviewsIfVisible(for: keys)
updatePreviewsIfVisible(state: state, tableView: tableView, for: keys)
}

if let key = state.focusSidebarKey, let indexPath = dataSource.indexPath(for: key) {
let isVisible = parentDelegate?.isSidebarVisible ?? false
tableView.selectRow(at: indexPath, animated: isVisible, scrollPosition: .middle)
}

if state.changes.contains(.sidebarEditingSelection) {
deleteBarButton?.isEnabled = state.deletionEnabled
mergeBarButton?.isEnabled = state.mergingEnabled
}

if state.changes.contains(.filter) || state.changes.contains(.annotations) || state.changes.contains(.sidebarEditing) {
setupToolbar(to: state)
}
}
}

/// Updates `UIImage` of `SquareAnnotation` preview if the cell is currently on screen.
/// - parameter keys: Set of keys to update.
private func updatePreviewsIfVisible(for keys: Set<String>) {
let cells = tableView.visibleCells.compactMap({ $0 as? AnnotationCell }).filter({ keys.contains($0.key) })

for cell in cells {
let image = viewModel.state.previewCache.object(forKey: (cell.key as NSString))
cell.updatePreview(image: image)
}
}

/// Reloads tableView if needed, based on new state. Calls completion either when reloading finished or when there was no reload.
/// - parameter state: Current state.
/// - parameter completion: Called after reload was performed or even if there was no reload.
private func reloadIfNeeded(for state: PDFReaderState, completion: @escaping () -> Void) {
if state.document.pageCount == 0 {
DDLogWarn("AnnotationsViewController: trying to reload empty document")
completion()
return
}

let isVisible = parentDelegate?.isSidebarVisible ?? false
/// Reloads tableView if needed, based on new state. Calls completion either when reloading finished or when there was no reload.
/// - parameter state: Current state.
/// - parameter completion: Called after reload was performed or even if there was no reload.
func reloadIfNeeded(for state: PDFReaderState, isVisible: Bool, completion: @escaping () -> Void) {
if state.document.pageCount == 0 {
DDLogWarn("AnnotationsViewController: trying to reload empty document")
completion()
return
}

if state.changes.contains(.annotations) {
if state.changes.contains(.sidebarEditing) {
tableView.setEditing(state.sidebarEditingEnabled, animated: false)
tableView.setEditing(state.sidebarEditingEnabled, animated: isVisible)
}
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = NSDiffableDataSourceSnapshot<Int, PDFReaderState.AnnotationKey>()
snapshot.appendSections([0])
snapshot.appendItems(state.sortedKeys)
if let keys = state.updatedAnnotationKeys {
snapshot.reloadItems(keys)

if state.changes.contains(.library) {
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = NSDiffableDataSourceSnapshot<Int, PDFReaderState.AnnotationKey>()
snapshot.reloadSections([0])
dataSource.apply(snapshot, animatingDifferences: isVisible, completion: completion)
}
dataSource.apply(snapshot, animatingDifferences: isVisible, completion: completion)
return
}
return
}

if state.changes.contains(.interfaceStyle) {
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = dataSource.snapshot()
guard !snapshot.sectionIdentifiers.isEmpty else { return }
snapshot.reloadSections([0])
dataSource.apply(snapshot, animatingDifferences: false, completion: completion)
if state.changes.contains(.annotations) {
updateUI(state: state, animatedDifferences: isVisible, completion: completion)
return
}
return
}

if state.changes.contains(.selection) || state.changes.contains(.activeComment) {
if let keys = state.updatedAnnotationKeys {
updateQueue.sync { [unowned self] in
if state.changes.contains(.interfaceStyle) && dataSource.snapshot().numberOfItems > 0 {
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = dataSource.snapshot()
snapshot.reloadItems(keys)
dataSource.apply(snapshot, animatingDifferences: false)
snapshot.reconfigureItems(snapshot.itemIdentifiers)
dataSource.apply(snapshot, animatingDifferences: false, completion: completion)
}
return
}

updateCellHeight()
focusSelectedCell()

if state.changes.contains(.sidebarEditing) {
tableView.setEditing(state.sidebarEditingEnabled, animated: isVisible)
if state.changes.contains(.selection) || state.changes.contains(.activeComment) {
var keys = state.updatedAnnotationKeys ?? []
if let key = state.selectedAnnotationKey, !keys.contains(key) {
keys.append(key)
}
if !keys.isEmpty {
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = dataSource.snapshot()
snapshot.reconfigureItems(keys)
dataSource.apply(snapshot, animatingDifferences: false) { [weak self] in
guard let self else { return }
focusSelectedCell()
completion()
}
}
return
}
}

completion()
return
}

if state.changes.contains(.library) {
tableView.reloadData()
}

if state.changes.contains(.sidebarEditing) {
tableView.setEditing(state.sidebarEditingEnabled, animated: true)
/// Updates `UIImage` of `SquareAnnotation` preview if the cell is currently on screen.
/// - parameter state: Current state.
/// - parameter tableView: Table view
/// - parameter keys: Set of keys to update.
func updatePreviewsIfVisible(state: PDFReaderState, tableView: UITableView, for keys: Set<String>) {
let cells = tableView.visibleCells.compactMap({ $0 as? AnnotationCell }).filter({ keys.contains($0.key) })
for cell in cells {
let image = state.previewCache.object(forKey: (cell.key as NSString))
cell.updatePreview(image: image)
}
}

completion()
}

/// Updates tableView layout in case any cell changed height.
private func updateCellHeight() {
UIView.setAnimationsEnabled(false)
tableView.beginUpdates()
tableView.endUpdates()
UIView.setAnimationsEnabled(true)
}

/// Scrolls to selected cell if it's not visible.
Expand Down Expand Up @@ -323,6 +323,7 @@ final class PDFAnnotationsViewController: UIViewController {
}

guard let boundingBoxConverter, let pdfAnnotationsCoordinatorDelegate = coordinatorDelegate else { return }
let reconfiguringForSameAnnotation = annotation.key == cell.key
cell.setup(
with: annotation,
text: text,
Expand All @@ -339,10 +340,13 @@ final class PDFAnnotationsViewController: UIViewController {
pdfAnnotationsCoordinatorDelegate: pdfAnnotationsCoordinatorDelegate,
state: state
)
let actionSubscription = cell.actionPublisher.subscribe(onNext: { [weak self] action in
self?.perform(action: action, annotation: annotation)
})
_ = cell.disposeBag?.insert(actionSubscription)
if !reconfiguringForSameAnnotation {
let actionSubscription = cell.actionPublisher.subscribe(onNext: { [weak self] action in
self?.perform(action: action, annotation: annotation)
})
_ = cell.disposeBag?.insert(actionSubscription)
}
// Otherwise, reconfigured cells do not have their prepareForReuse method called, so observing is already set up.

func loadPreview(for annotation: PDFAnnotation, state: PDFReaderState) -> UIImage? {
let preview = state.previewCache.object(forKey: (annotation.key as NSString))
Expand Down
17 changes: 7 additions & 10 deletions Zotero/Scenes/Master/Collections/Views/ExpandableCollectionsCollectionViewHandler.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,21 +47,18 @@ final class ExpandableCollectionsCollectionViewHandler: NSObject {
collectionView.delegate = self
collectionView.dropDelegate = self
dataSource = createDataSource(for: collectionView)
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = NSDiffableDataSourceSnapshot<Int, Collection>()
snapshot.appendSections([collectionsSection])
dataSource.apply(snapshot, animatingDifferences: false)
}

func createDataSource(for collectionView: UICollectionView) -> UICollectionViewDiffableDataSource<Int, Collection> {
let registration = cellRegistration

let dataSource = UICollectionViewDiffableDataSource<Int, Collection>(collectionView: collectionView, cellProvider: { collectionView, indexPath, collection in
return UICollectionViewDiffableDataSource<Int, Collection>(collectionView: collectionView, cellProvider: { collectionView, indexPath, collection in
return collectionView.dequeueConfiguredReusableCell(using: registration, for: indexPath, item: collection)
})

updateQueue.sync {
var snapshot = NSDiffableDataSourceSnapshot<Int, Collection>()
snapshot.appendSections([collectionsSection])
dataSource.apply(snapshot, animatingDifferences: false)
}

return dataSource
}
}

Expand Down

0 comments on commit 583a83c

Please sign in to comment.