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

fix: import file only once to document #1158

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

letsfindaway
Copy link
Collaborator

This PR fixes #1156 where the first page of a PDF document was repeated at the end when importing the PDF by dropping it to the board.

  • when importing a file to the board, the file was offered to all import adapters
  • if multiple import adapters support the same file extension, then the file was imported multiple times
  • this was especially true for pdf documents, where in some environments also the image adapter supported rendering a pdf document (preview image of first page)
  • this commit leaves the loop iterating over the adapters once one of them has successfully imported the file

@sebojolais: could you test this PR whether it fixes the problem for you?

- when importing a file to the board, the file was offered to
  all import adapters
- if multiple import adapters support the same file extension, then
  the file was imported multiple times
- this was especially true for pdf documents, where in some environments
  also the image adapter supported rendering a pdf document
  (preview image of first page)
- this commit leaves the loop iterating over the adapters once one of
  them has successfully imported the file
@sebojolais
Copy link
Contributor

Hi,
Thank you for your PR.
It fixes the issue #1156 I observed in my side.
tested with Qt 5.15 and 6.7

@sebojolais
Copy link
Contributor

sebojolais commented Nov 6, 2024

One question about this PR:

It make sense to let only one adapter to import the document.
But how we can be sure is the best adapter for this format ?

In other words, with a example:
Currently, the adapters list is :
image

The PR fixes the #1156 issue because the PDF adapter is before the Image adapter in the list.
If it was not the case, the PR should not fixes the #1156 issue.
But it will be always the case because the order of the adapters is defined in the constructor:

UBImportDocument* documentImport = new UBImportDocument(this);
mImportAdaptors.append(documentImport);
UBImportDocumentSetAdaptor *documentSetImport = new UBImportDocumentSetAdaptor(this);
mImportAdaptors.append(documentSetImport);
UBImportPDF* pdfImport = new UBImportPDF(this);
mImportAdaptors.append(pdfImport);
UBImportImage* imageImport = new UBImportImage(this);
mImportAdaptors.append(imageImport);
UBImportCFF* cffImport = new UBImportCFF(this);
mImportAdaptors.append(cffImport);

So I try to answer to my question: But how we can be sure is the best adapter for this format ?
We can not be sure, but the double adapters import happens only for PDF in a specific environment. And in this case the good PDF adapter will be always used first.

@letsfindaway
Copy link
Collaborator Author

letsfindaway commented Nov 7, 2024

So I try to answer to my question: But how we can be sure is the best adapter for this format ?
We can not be sure, but the double adapters import happens only for PDF in a specific environment. And in this case the good PDF adapter will be always used first.

Yes, exactly. This is a very good question, and I also asked it to myself. To make the answer a little bit more generic:

To find the right adapter when multiple adapters would offer to handle a file format we can have two approaches:

  • Some kind of voting, where each adapter expresses a "suitability" and we choose the highest.
  • Or a fixed sequence, which orders the preferred adapter first.

The current code guarantees a fixed order, so the second approach works well. (Side note: a registration approach where each adapter offers its capabilities to the document manager in an unspecified order would require the first approach.) If we care to order more specific adapters before more generic ones, then this solves the problem. The current sequence already fulfills this condition.

And as you said, PDF is currently the only one where this occurs. And the image adapter is the only one where the list of supported file formats can vary. All others return a fixed string list.

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.

2 participants