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

Page thumbnails in PDF reader sidebar #815

Merged
merged 8 commits into from
Dec 14, 2023

Conversation

michalrentka
Copy link
Contributor

@michalrentka michalrentka commented Dec 7, 2023

Closes #742

@michalrentka michalrentka requested a review from mvasilak December 8, 2023 09:11
Copy link
Contributor

@mvasilak mvasilak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason the new files are named e.g. PdfThumbnailsViewController instead of PDFThumbnailsViewController?

Zotero/Models/Files.swift Outdated Show resolved Hide resolved
annotationsController.coordinatorDelegate = self.coordinatorDelegate
annotationsController.boundingBoxConverter = self.boundingBoxConverter
self.addChild(annotationsController)
let annotationsController = AnnotationsViewController(viewModel: viewModel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we create several controllers when the document appears, that may not even be used at all, maybe we can convert them to lazy load, in a future update. Or maybe just pre-load the first, or some.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this to be pretty much the same as UITabbarController which allocates all controllers ahead too. But we can definitely improve this by lazy loading.

@michalrentka
Copy link
Contributor Author

michalrentka commented Dec 11, 2023

Is there a reason the new files are named e.g. PdfThumbnailsViewController instead of PDFThumbnailsViewController?

Just my memory I'm afraid :), I'll rename to uppercase.

Fixed a56ffdb

@michalrentka michalrentka merged commit f1f9c77 into zotero:master Dec 14, 2023
1 check passed
@michalrentka michalrentka deleted the thumbnails branch December 14, 2023 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Page thumbnails tab in sidebar
2 participants