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

refactor: unified thumbnails for board and document mode #1162

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

letsfindaway
Copy link
Collaborator

@letsfindaway letsfindaway commented Nov 12, 2024

This PR is the result of my considerations about a rework of the thumbnail architecture initially started here: letsfindaway#190.

I have also created a wiki page describing the new architecture here: https://github.com/letsfindaway/OpenBoard/wiki/New-Thumbnail-Architecture. Let me just describe the core ideas here:

  • Currently Board mode and Document mode have their own classes and implementations for the thumbnails. As both modes have to be in sync, there is a lot of error-prone code for synchronizing the thumbnails. just one example: if you add a page in Board mode, then you have to care that the thumbnail for that page is also created and kept up-to-date for the Document mode and vice versa. So the core idea is to have only one UBThumbnail class for both modes and to even use the same instances on the same UBThumbnailScene. Switching between modes then simply means re-arranging the thumbnails. You do not have to care about synchronization at all.
  • The UBThumbnailScene is managed by a UBDocument class. This class has functions to create, delete, move and copy pages and takes care to invoke the UBPersistenceManager as well as to handle the thumbnails associated with the affected pages. One could cache instances of UBDocument together with the associated thumbnails. This makes switching between documents very fast.
    Note Currently all UBDocument instances are cached during a session. It is TBD whether a more sophisticated caching strategy is necessary and how it should look like.
    Edit I added a second commit implementing a ver simple caching strategy: Keep UBDocument instances and therefore thumbnails for documents which have been opened in Board mode in the current session.
  • The different arrangement of the thumbnails in Board and Document mode is reflected in a UBThumbnailArranger with specific derived classes for the two modes.

The resulting code is much easier to understand and to maintain. And in fact it needs 500 lines of code less than before. Also the memory consumption for thumbnails is halved, as we need only one instance for Board and Document mode. Rearrangement of thumbnails is fast (50ms for 2000 thumbnails on my computer), so the holdoff timer to reduce frequency of rearrangements is not necessary.

The UBThumbnailScene uses deferred loading of thumbnail. This means that the user has no waiting time when selecting a document with many pages in Document mode. They can immediately start working with the document, even if more thumbnails are still loaded in the background.

I think that this PR is also a very good preparation for "Board thumbnails look like the last state of the view" and even more "Multiple documents opened side-by-side". The first idea benefits from the fact that synchronization is no longer necessary. The second one from the fact, that we have one thumbnail scene per document.

I also tested various aspects of the new implementation. Here is my test list: letsfindaway#190 (comment)

Here some more technical points:

  • Use the same thumbnail items and the same thumbnail scene for Board and Document mode. Thumbnails are just rearranged depending on the mode.
  • Create a UBDocument class for page handling. This class has functions for creating, deleting, moving and copying of pages. It takes care to call the proper functions on the UBPersistenceManager and to manage the thumbnail items. All other code shall only use these functions.
  • Use a specific, concrete derived class of an abstract UBThumbnailArranger to handle the differences between Board and Document mode.
  • Adapt the UBDocumentController and UBBoardController to the new thumbnails.
  • Adapt the UBDocumentThumbnailsView and UBBoardThumbnails view to the new thumbnails.
  • Remove cross-referencing includes in thumbnail views.
  • Remove thumbnail pixmap handling from UBDocumentContainer.
  • Fix keyboard selection in Document mode.
  • Ensure last selected thumbnails stays visible when changing size.
  • Re-enable the "Delete" and "Duplicate" buttons appearing when hovering over a thumbnail in Document mode. This was implemented, but did not work due to missing event forwarding.

It is probably worth to give just one example from the UBBoardController::addScene():

Before

    UBPersistenceManager::persistenceManager()->createDocumentSceneAt(selectedDocument(), mActiveSceneIndex+1);
    emit addThumbnailRequired(selectedDocument(), mActiveSceneIndex+1);
    if (UBApplication::documentController->selectedDocument() == selectedDocument())
    {
        UBApplication::documentController->insertThumbPage(mActiveSceneIndex +1);
        UBApplication::documentController->reloadThumbnails();
    }

After

    auto document = UBDocument::getDocument(selectedDocument());
    document->createPage(mActiveSceneIndex + 1);

@letsfindaway letsfindaway force-pushed the refactor-unified-thumbnails branch 3 times, most recently from 01ddb51 to 07f073c Compare November 14, 2024 15:44
- use the same thumbnail items and the same thumbnail scene for Board
  and Document mode. Thumbnails are just rearranged depending on the
mode
- create a UBDocument class for page handling. This class has functions
  for creating, deleting, moving and copying of pages. It takes care to
  call the proper functions on the UBPersistenceManager and to manage
the
  thumbnail items. All other code shall only use these functions
- use a specific, concrete derived class of an abstract
UBThumbnailArranger
  to handle the differences between Board and Document mode
- adapt the UBDocumentController and UBBoardController to the new
  thumbnails
- adapt the UBDocumentThumbnailsView and UBBoardThumbnails view to the
  new thumbnails
- remove cross-referencing includes in thumbnail views
- remove thumbnail pixmap handling from UBDocumentContainer
- fix keyboard selection in Document mode
- ensure last selected thumbnails stays visible when changing size
- cache thumbnails for documents which have been opened in board mode
@letsfindaway letsfindaway force-pushed the refactor-unified-thumbnails branch from 07f073c to 9e34c19 Compare November 17, 2024 14:13
- reload thumbnail pixmap from file when a scene is persisted and
  the associated thumbnail was hidden and therefore not live updated
@letsfindaway
Copy link
Collaborator Author

letsfindaway commented Nov 22, 2024

I just added a commit fixing issue #1171 which also occurred on this branch.

When the thumbnail palette is hidden, thumbnails are not live updated and are therefore outdated when the palette is shown later. We now reload a thumbnail from the thumbnail file when a scene was persisted and the thumbnail file was updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant