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.

Sign up for GitHub

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

Jump to bottom

Fixed reentrancy issue in pdf reader #733

Merged
merged 2 commits into from
Jul 31, 2023
Merged

Fixed reentrancy issue in pdf reader #733

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8a1338
Fixed reentrancy issue in pdf reader
michalrentka Jul 28, 2023
4aca643
Style fix
michalrentka Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions Zotero/Controllers/Architecture/ViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ protocol ViewModelActionHandler {
}

extension ViewModelActionHandler {
func update(viewModel: ViewModel<Self>, action: (inout State) -> Void) {
viewModel.update(action: action)
func update(viewModel: ViewModel<Self>, notifyListeners: Bool = true, action: (inout State) -> Void) {
viewModel.update(notifyListeners: notifyListeners, action: action)
}
}

Expand Down Expand Up @@ -136,10 +136,13 @@ final class ViewModel<Handler: ViewModelActionHandler>: ObservableObject {
})
}

fileprivate func update(action: (inout Handler.State) -> Void) {
fileprivate func update(notifyListeners: Bool, action: (inout Handler.State) -> Void) {
var state = self.state
state.cleanup()
action(&state)

guard notifyListeners else { return }

inMainThread {
self.stateObservable.accept(state)
self.objectWillChange.send()
Expand Down
2 changes: 1 addition & 1 deletion Zotero/Scenes/Detail/PDF/ViewModels/PDFReaderActionHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ final class PDFReaderActionHandler: ViewModelActionHandler, BackgroundDbProcessi
self.set(highlightText: highlight, key: key, viewModel: viewModel)

case .parseAndCacheComment(let key, let comment):
self.update(viewModel: viewModel) { state in
self.update(viewModel: viewModel, notifyListeners: false) { state in
state.comments[key] = self.htmlAttributedStringConverter.convert(text: comment, baseAttributes: [.font: viewModel.state.commentFont])
}

Expand Down
176 changes: 109 additions & 67 deletions Zotero/Scenes/Detail/PDF/Views/AnnotationsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,28 @@ final class AnnotationsViewController: UIViewController {
})

case .options(let sender):
self.coordinatorDelegate?.showCellOptions(for: annotation, userId: self.viewModel.state.userId, library: self.viewModel.state.library, sender: sender, userInterfaceStyle: self.viewModel.state.interfaceStyle,
saveAction: { [weak self] key, color, lineWidth, pageLabel, updateSubsequentLabels, highlightText in
self?.viewModel.process(action: .updateAnnotationProperties(key: key.key, color: color, lineWidth: lineWidth, pageLabel: pageLabel,
updateSubsequentLabels: updateSubsequentLabels, highlightText: highlightText))
},
deleteAction: { [weak self] key in
self?.viewModel.process(action: .removeAnnotation(key))
})
self.coordinatorDelegate?.showCellOptions(
for: annotation,
userId: self.viewModel.state.userId,
library: self.viewModel.state.library,
sender: sender,
userInterfaceStyle: self.viewModel.state.interfaceStyle,
saveAction: { [weak self] key, color, lineWidth, pageLabel, updateSubsequentLabels, highlightText in
self?.viewModel.process(
action: .updateAnnotationProperties(
key: key.key,
color: color,
lineWidth: lineWidth,
pageLabel: pageLabel,
updateSubsequentLabels: updateSubsequentLabels,
highlightText: highlightText
)
)
},
deleteAction: { [weak self] key in
self?.viewModel.process(action: .removeAnnotation(key))
}
)

case .setComment(let comment):
self.viewModel.process(action: .setComment(key: annotation.key, comment: comment))
Expand Down Expand Up @@ -284,14 +298,26 @@ final class AnnotationsViewController: UIViewController {
preview = loadPreview()

case .note, .highlight:
preview = nil
comment = .init(attributedString: self.loadAttributedComment(for: annotation), isActive: state.selectedAnnotationCommentActive)
preview = nil
}

if let boundingBoxConverter = self.boundingBoxConverter, let pdfAnnotationsCoordinatorDelegate = coordinatorDelegate {
cell.setup(with: annotation, comment: comment, preview: preview, selected: selected, availableWidth: PDFReaderLayout.sidebarWidth, library: state.library,
isEditing: state.sidebarEditingEnabled, currentUserId: self.viewModel.state.userId, displayName: self.viewModel.state.displayName, username: self.viewModel.state.username,
boundingBoxConverter: boundingBoxConverter, pdfAnnotationsCoordinatorDelegate: pdfAnnotationsCoordinatorDelegate, state: state)
cell.setup(
with: annotation,
comment: comment,
preview: preview,
selected: selected,
availableWidth: PDFReaderLayout.sidebarWidth,
library: state.library,
isEditing: state.sidebarEditingEnabled,
currentUserId: self.viewModel.state.userId,
displayName: self.viewModel.state.displayName,
username: self.viewModel.state.username,
boundingBoxConverter: boundingBoxConverter,
pdfAnnotationsCoordinatorDelegate: pdfAnnotationsCoordinatorDelegate,
state: state
)
}
let actionSubscription = cell.actionPublisher.subscribe(onNext: { [weak self] action in
self?.perform(action: action, annotation: annotation)
Expand Down Expand Up @@ -346,10 +372,16 @@ final class AnnotationsViewController: UIViewController {
}
}

self.coordinatorDelegate?.showFilterPopup(from: barButton, filter: self.viewModel.state.filter, availableColors: sortedColors, availableTags: sortedTags,
userInterfaceStyle: self.viewModel.state.interfaceStyle, completed: { [weak self] filter in
self?.viewModel.process(action: .changeFilter(filter))
})
self.coordinatorDelegate?.showFilterPopup(
from: barButton,
filter: self.viewModel.state.filter,
availableColors: sortedColors,
availableTags: sortedTags,
userInterfaceStyle: self.viewModel.state.interfaceStyle,
completed: { [weak self] filter in
self?.viewModel.process(action: .changeFilter(filter))
}
)
}

// MARK: - Setups
Expand Down Expand Up @@ -445,28 +477,34 @@ final class AnnotationsViewController: UIViewController {
}

self.dataSource.commitEditingStyle = { [weak self] editingStyle, indexPath in
guard let self = self, !self.viewModel.state.sidebarEditingEnabled && editingStyle == .delete,
let key = self.dataSource.itemIdentifier(for: indexPath), key.type == .database else { return }
guard let self,
!self.viewModel.state.sidebarEditingEnabled && editingStyle == .delete,
let key = self.dataSource.itemIdentifier(for: indexPath),
key.type == .database
else { return }
self.viewModel.process(action: .removeAnnotation(key))
}
}

private func setupSearchController() {
let insets = UIEdgeInsets(top: PDFReaderLayout.searchBarVerticalInset,
left: PDFReaderLayout.annotationLayout.horizontalInset,
bottom: PDFReaderLayout.searchBarVerticalInset - PDFReaderLayout.cellSelectionLineWidth,
right: PDFReaderLayout.annotationLayout.horizontalInset)
let insets = UIEdgeInsets(
top: PDFReaderLayout.searchBarVerticalInset,
left: PDFReaderLayout.annotationLayout.horizontalInset,
bottom: PDFReaderLayout.searchBarVerticalInset - PDFReaderLayout.cellSelectionLineWidth,
right: PDFReaderLayout.annotationLayout.horizontalInset
)

var frame = self.tableView.frame
frame.size.height = 65

let searchBar = SearchBar(frame: frame, insets: insets, cornerRadius: 10)
searchBar.text.observe(on: MainScheduler.instance)
.debounce(.milliseconds(150), scheduler: MainScheduler.instance)
.subscribe(with: self, onNext: { `self`, text in
self.viewModel.process(action: .searchAnnotations(text))
})
.disposed(by: self.disposeBag)
searchBar.text
.observe(on: MainScheduler.instance)
.debounce(.milliseconds(150), scheduler: MainScheduler.instance)
.subscribe(with: self, onNext: { `self`, text in
self.viewModel.process(action: .searchAnnotations(text))
})
.disposed(by: self.disposeBag)
self.tableView.tableHeaderView = searchBar
}

Expand All @@ -478,29 +516,34 @@ final class AnnotationsViewController: UIViewController {

private func setupKeyboardObserving() {
NotificationCenter.default
.keyboardWillShow
.observe(on: MainScheduler.instance)
.subscribe(with: self, onNext: { `self`, notification in
if let data = notification.keyboardData {
self.setupTableView(with: data)
}
})
.disposed(by: self.disposeBag)
.keyboardWillShow
.observe(on: MainScheduler.instance)
.subscribe(with: self, onNext: { `self`, notification in
if let data = notification.keyboardData {
self.setupTableView(with: data)
}
})
.disposed(by: self.disposeBag)
mvasilak marked this conversation as resolved.
Show resolved Hide resolved

NotificationCenter.default
.keyboardWillHide
.observe(on: MainScheduler.instance)
.subscribe(with: self, onNext: { `self`, notification in
if let data = notification.keyboardData {
self.setupTableView(with: data)
}
})
.disposed(by: self.disposeBag)
.keyboardWillHide
.observe(on: MainScheduler.instance)
.subscribe(with: self, onNext: { `self`, notification in
if let data = notification.keyboardData {
self.setupTableView(with: data)
}
})
.disposed(by: self.disposeBag)
}

private func setupToolbar(to state: PDFReaderState) {
self.setupToolbar(filterEnabled: (state.databaseAnnotations?.count ?? 0) > 1, filterOn: (state.filter != nil), editingEnabled: state.sidebarEditingEnabled,
deletionEnabled: state.deletionEnabled, mergingEnabled: state.mergingEnabled)
self.setupToolbar(
filterEnabled: (state.databaseAnnotations?.count ?? 0) > 1,
filterOn: (state.filter != nil),
editingEnabled: state.sidebarEditingEnabled,
deletionEnabled: state.deletionEnabled,
mergingEnabled: state.mergingEnabled
)
}

private func setupToolbar(filterEnabled: Bool, filterOn: Bool, editingEnabled: Bool, deletionEnabled: Bool, mergingEnabled: Bool) {
Expand All @@ -524,11 +567,11 @@ final class AnnotationsViewController: UIViewController {
let delete = UIBarButtonItem(title: L10n.delete, style: .plain, target: nil, action: nil)
delete.isEnabled = deletionEnabled
delete.rx.tap
.subscribe(with: self, onNext: { `self`, _ in
guard self.viewModel.state.sidebarEditingEnabled else { return }
self.viewModel.process(action: .removeSelectedAnnotations)
})
.disposed(by: self.disposeBag)
.subscribe(with: self, onNext: { `self`, _ in
guard self.viewModel.state.sidebarEditingEnabled else { return }
self.viewModel.process(action: .removeSelectedAnnotations)
})
.disposed(by: self.disposeBag)
items.append(delete)
self.deleteBarButton = delete
} else if filterEnabled {
Expand All @@ -539,18 +582,18 @@ final class AnnotationsViewController: UIViewController {
let filter = UIBarButtonItem(image: UIImage(systemName: filterImageName), style: .plain, target: nil, action: nil)
filter.rx.tap
.subscribe(with: self, onNext: { `self`, _ in
self.showFilterPopup(from: filter)
})
.disposed(by: self.disposeBag)
self.showFilterPopup(from: filter)
})
.disposed(by: self.disposeBag)
items.insert(filter, at: 0)
}

let select = UIBarButtonItem(title: (editingEnabled ? L10n.done : L10n.select), style: .plain, target: nil, action: nil)
select.rx.tap
.subscribe(with: self, onNext: { `self`, _ in
self.viewModel.process(action: .setSidebarEditingEnabled(!editingEnabled))
})
.disposed(by: self.disposeBag)
.subscribe(with: self, onNext: { `self`, _ in
self.viewModel.process(action: .setSidebarEditingEnabled(!editingEnabled))
})
.disposed(by: self.disposeBag)
items.append(select)

self.toolbar.items = items
Expand All @@ -560,20 +603,19 @@ final class AnnotationsViewController: UIViewController {
extension AnnotationsViewController: UITableViewDelegate, UITableViewDataSourcePrefetching {
func tableView(_ tableView: UITableView, prefetchRowsAt indexPaths: [IndexPath]) {
let keys = indexPaths.compactMap({ self.dataSource.itemIdentifier(for: $0) })
.filter({ key in
guard let annotation = self.viewModel.state.annotation(for: key) else { return false }
switch annotation.type {
case .image, .ink: return true
case .note, .highlight: return false
}
})
.map({ $0.key })
.filter({ key in
guard let annotation = self.viewModel.state.annotation(for: key) else { return false }
switch annotation.type {
case .image, .ink: return true
case .note, .highlight: return false
}
})
.map({ $0.key })
self.viewModel.process(action: .requestPreviews(keys: keys, notify: false))
}

func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
guard let key = self.dataSource.itemIdentifier(for: indexPath) else { return }

if self.viewModel.state.sidebarEditingEnabled {
self.viewModel.process(action: .selectAnnotationDuringEditing(key))
} else {
Expand Down