Skip to content

Conversation

@benwaffle
Copy link

@benwaffle benwaffle commented May 8, 2025

Resolves: #27136

This PR adds (1) a QuickLook plugin, (2) a thumbnail provider plugin, (3) embeds a .pdf of each score inside the .mscz when saving.

TODO:

  • add codesigning of the .appex plugins for release
  • score.pdf probably shouldn't be in the Thumbnails/ directory
  • delete unused code in ThumbnailProvider.swift

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@benwaffle
Copy link
Author

@cbjeukendrup can I get a review, when you get the chance?

@cbjeukendrup
Copy link
Member

@benwaffle yes sure, I'll leave some comments asap!
Actually I had already taken a quick look, and it definitely seems nice! It's an interesting idea to use Swift Package Manager. On the other hand, I'm still investigating a bit what the advantages and disadvantages are, compared to using CMake's Swift support. Also, I'm still investigating whether the install step is the best moment to include and sign the plugins, or that we can do it during the build phase, which might have the advantage that some steps could be done only when input files have been changed.

Anyway, that's the status update, and I'll let you know when I have any conclusion!

@cbjeukendrup cbjeukendrup self-requested a review May 14, 2025 07:59
@benwaffle
Copy link
Author

The reason I used it was because the plugins have a dependency on the ZIPFoundation library. If not using SPM we need some way to download it.

Copy link
Member

@cbjeukendrup cbjeukendrup left a comment

Choose a reason for hiding this comment

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

In the end, I decided that it's probably okay to leave the CMake code as it is; any change is unlikely to make it less complicated. It might become more robust, but if it already works well enough (also with re-building after changing source files), there is no reason to spend much more time on making it more robust.

I did still find some comments about other things though.

public:
Inject<IEngravingElementsProvider> engravingElementsProvider = { this };
Inject<context::IGlobalContext> globalContext = { this };
Inject<project::INotationWritersRegister> writers = { this };
Copy link
Member

Choose a reason for hiding this comment

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

This turns out to be problematic, because it introduces a cyclic dependency between the engraving and project/notation modules.

One solution would be to move the pdf creation implementation into the engraving module, and call that implementation in PdfWriter as well as MscSaver::writeMscz.

Another option might be to create an interface class in the engraving module, and implement this interface in some other module. A bit like what's suggested here, as a solution for a similar problem: #27324 (comment). But that interface will have to take EngravingProject and not INotation, and globalContext()->currentNotation() cannot be used, because that only represents the UI state, but there is no guarantee that the engraving project being written is indeed the current notation in the UI.

Copy link
Author

@benwaffle benwaffle May 17, 2025

Choose a reason for hiding this comment

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

Hmm...I'm not familiar with the code enough to tell which one is better. What do you think? What's the difference between "notation" and "engraving" anyway? Looks like "engraving" should be buildable separately without Qt? Does PDF creation belong in engraving?

Copy link
Author

Choose a reason for hiding this comment

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

Is it ok to use QPdfWriter in src/engraving? I guess we won't have PDFs if NO_QT_SUPPORT is set.

Copy link
Author

Choose a reason for hiding this comment

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

I figured out how to do it without "notation" module. Take a look.

@benwaffle benwaffle force-pushed the quicklook branch 3 times, most recently from fb886dd to 386eddb Compare May 17, 2025 07:00
@cbjeukendrup cbjeukendrup self-requested a review May 19, 2025 06:54
@benwaffle
Copy link
Author

I think codesigning for release builds actually works already because it uses --deep.

@cbjeukendrup everything looks good from here. What else do we need to get this merged?

@benwaffle benwaffle mentioned this pull request Jun 3, 2025
8 tasks
@cbjeukendrup
Copy link
Member

Rebased to fix conflicts; still planning to merge this, but there are still some file size concerns about the embedded PDF. But I'm working on a solution for that myself.

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.

Add a QuickLook plugin on macOS to quickly preview .mscz files

2 participants