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

Support multiple open items #829

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Support multiple open items #829

wants to merge 5 commits into from

Conversation

mvasilak
Copy link
Contributor

@mvasilak mvasilak commented Jan 3, 2024 •
edited
Loading

Adds support for multiple open items.

  • Keep track of open items in order of most recently opened
  • Show in items navigation bar button that restores last item and shows count of currently open items
  • Save/restore state of open items
  • Add support for PDF items
  • Add support for note items
  • Show in presented item navigation bar button menu that allows to quickly switch to another open item
  • Allow user to change order of presented open items (only move first & last currently)

In consideration

  • Support separate open items for multiple windows
  • Opening an already opened item in another window, should switch to that

Previously discussed in #782

@mvasilak mvasilak marked this pull request as draft January 3, 2024 12:42
@mvasilak mvasilak self-assigned this Jan 3, 2024
@mvasilak mvasilak force-pushed the new-multiple-open-items branch 2 times, most recently from cec3c1b to fcaffcf Compare January 16, 2024 09:28
@mvasilak mvasilak force-pushed the new-multiple-open-items branch 2 times, most recently from 33ad637 to b384d53 Compare February 22, 2024 10:21
@mvasilak mvasilak force-pushed the new-multiple-open-items branch 2 times, most recently from 1edc31b to 01da13a Compare February 26, 2024 13:55
@mvasilak mvasilak force-pushed the new-multiple-open-items branch 3 times, most recently from e918514 to 21deedc Compare April 15, 2024 11:02
@mvasilak mvasilak marked this pull request as ready for review April 18, 2024 12:57
@mvasilak mvasilak changed the title WIP Support multiple open items Support multiple open items Apr 18, 2024
@mvasilak
Copy link
Contributor Author

mvasilak commented Apr 18, 2024 •
edited
Loading

@michalrentka @dstillman
this PR is ready for some more eyes and perhaps beta testing. It has the underlying facility to support:

  • multiple open items
  • switching between open items
  • opening new items
  • closing existing items
  • moving open items to another position (user index)
  • keeping track of last opened date for each item
  • opening an item in its corresponding window/scene if it's already open

It currently supports PDF documents and notes, and should be straight-forward to add support for e.g. web snapshots and ePubs.

UI-wise, it includes a bare minimum implementation using simple UIMenus, with these features:

  • In the items screen, a bar button appears with a number indicating the number of open items.
Screenshot 2024-04-18 at 16 10 00
  • Tapping the button, will open the last opened item.
  • Tapping and holding will show a menu that allows opening specific item, or closing all open items.
Screenshot 2024-04-18 at 16 13 57
  • Similar bar button appears in the PDF document/note screen.
Screenshot 2024-04-18 at 16 15 22 Screenshot 2024-04-18 at 16 16 01
  • Tapping the button will always show a menu. It contains all the open items as before, but also adds a sub-menu for the current item. Said sub-menu allows to close current item, move to start (if not already), move to end (if not already), close other items (if other items are open).
Screenshot 2024-04-18 at 16 18 24

Next (possible) steps (in separate PRs) are (a) an actual tab bar and (b) instead of using UIMenus create a custom view that allows e.g. closing any item and re-ordering.

Feedback regarding icons and copy used will be much appreciated.

michalrentka
michalrentka requested changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@michalrentka michalrentka left a comment

Choose a reason for hiding this comment

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

Code-wise there are just some minor mostly style-related adjustments. After TO-DOs are resolved this looks good. I'll spend some more time testing this on device, but to me it looks good for at least internal user testing.

Zotero/Extensions/NSUserActivity+Activities.swift Outdated Show resolved Hide resolved
Zotero/Scenes/AppCoordinator.swift Outdated Show resolved Hide resolved
Zotero/Scenes/AppCoordinator.swift Outdated Show resolved Hide resolved
Zotero/Scenes/Detail/DetailCoordinator.swift Outdated Show resolved Hide resolved
Zotero/Scenes/Detail/Items/Views/ItemsViewController.swift Outdated Show resolved Hide resolved
Zotero/Scenes/Detail/PDF/Views/PDFReaderViewController.swift Show resolved Hide resolved
Zotero/Controllers/OpenItemsController.swift Outdated Show resolved Hide resolved
Zotero/Controllers/OpenItemsController.swift Show resolved Hide resolved
Zotero/Controllers/OpenItemsController.swift Outdated Show resolved Hide resolved
Zotero/Controllers/OpenItemsController.swift Outdated Show resolved Hide resolved
@michalrentka
Copy link
Contributor

Some bugs and smaller issues I caught:

  1. On iPhone, the square bar button item for tabs seems to have a lot of space around it
    bar button
  2. When I open multiple PDFs, then I scroll to some page on one, switch to another one and switch back to the original one, the original one is not on the page I scrolled to, but on page that it was when I opened it

@mvasilak
Copy link
Contributor Author

@michalrentka

1. On iPhone, the square bar button item for tabs seems to have a lot of space around it

It's the standard spacing for bar button items created with an image, it just seems like that because is next to another such button. We can use a custom view button instead, to decrease the image insets. Maybe we should consider using UIBarButtonItemGroup to group related buttons when there are space constraints.
Screenshot 2024-04-25 at 16 37 42

2. When I open multiple PDFs, then I scroll to some page on one, switch to another one and switch back to the original one, the original one is not on the page I scrolled to, but on page that it was when I opened it

Good catch, fixed in 33b5f63

@michalrentka
Copy link
Contributor

Looks good to me now, I didn't find new bugs yet. We should probably make an internal build for others to test or push it to beta. There are a ton of changes, so we should definitely test this thoroughly. @dstillman can decide.

@mvasilak
Copy link
Contributor Author

mvasilak commented May 8, 2024

@michalrentka rebased to master, to include the activity/scene title logic, and made some relevant improvements for the note editor.

@mvasilak mvasilak force-pushed the new-multiple-open-items branch 2 times, most recently from bd31e1f to 9faffc0 Compare August 9, 2024 13:04
@mvasilak mvasilak force-pushed the new-multiple-open-items branch 2 times, most recently from 429169c to bfbd058 Compare August 28, 2024 16:30
michalrentka
michalrentka reviewed Aug 31, 2024
View reviewed changes
Zotero/Controllers/OpenItemsController.swift Outdated Show resolved Hide resolved
Zotero/Controllers/OpenItemsController.swift
keysByLibraryIdentifier[libraryId] = keys
}
do {
let objects = try dbStorage.perform(request: ReadItemsWithKeysFromMultipleLibrariesDbRequest(keysByLibraryIdentifier: keysByLibraryIdentifier), on: .main)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this fails we'll end up with new keys in keysByLibraryIdentifier but token in itemsTokenBySessionIdentifier will be nil. Shouldn't we update keysByLibraryIdentifier only after successfully reading these objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

keysByLibraryIdentifier is a local variable that is populated as a parameter for the db request, it's not used outside of the function scope.

Zotero/Controllers/OpenItemsController.swift Outdated Show resolved Hide resolved
Zotero/Controllers/OpenItemsController.swift Outdated Show resolved Hide resolved
Zotero/Scenes/AppCoordinator.swift Show resolved Hide resolved
Zotero/Scenes/AppCoordinator.swift Show resolved Hide resolved
@mvasilak
Add support for multiple open items
6b89055 
Improve AppCoordinator code
Keep track of open items count in items navigation bar
Add ability to restore most recently opened item
Implement state restoration for open items
Allow switch to another open item via bar button menu
Use instant presenter to switch between open items
Maintain user order of open items
Add support for notes in open items
Validate open items when set on app launch
Observe open items for deletions
Add support for different open items per session
Improve DetailCoordinator presented item replacement for multiple items
Set user activity when new note is actually created
Show actions submenu for current item
Improve ItemsViewController right bar button items
Add icons item type icons to open items menu
Add close all action to open items menu
Simplify NoteEditorViewController open items button creation
Add PDFReaderViewController open items observer
Add close other items action to submenu for current item
Add getSessionIdentifier convenience property to UIViewController
Save user activity when open items change w/o current item change
Open items in their respective scene, if already open
Update copy
Improve NSUserActivity extension
Simplify DetailCoordinator
Improve open items bar button image creation
Improve open items bar button creation
Properly close PDF Reader when switching current open item
Properly close Note Editor when switching current open item
Improve note editor save callback for new notes
Improve note editor activity title update
Improve NoteEditorActionHandler
Refactor PDFReaderState
Improve PDFReaderActionHandler
Pass note title when showing note editor
Clarify NoteEditorState parameter names
@mvasilak
Make OpenItemsController restoration methods asynchronous
436b66b
@mvasilak
In OpenItemsController download remote or local changed remotely PDFs
3232c74
@mvasilak
Improve code
a615ebd
@mvasilak
Improve code
db16b14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalrentka michalrentka michalrentka approved these changes

Assignees

@mvasilak mvasilak

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mvasilak @michalrentka