MusicXmlViewer extension for sheet music #2752
Conversation
Signed-off-by: Benjamin Giesinger <benjamin.giesinger@pm.me>
Signed-off-by: Benjamin Giesinger <benjamin.giesinger@pm.me>
Signed-off-by: Benjamin Giesinger <benjamin.giesinger@pm.me>
bneumann
force-pushed
the
feat/musicXmlViewer
branch
from
18a7bc6 to
4cdd9fe
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
| 'application/vnd.recordare.musicxml+xml', | ||
| // this registers a binary file for the sheet viewer. | ||
| // The Viewer itself checks if it can make sense of it | ||
| 'application/octet-stream', |
There was a problem hiding this comment.
This should not be there.
the two mime above should be enough :)
There was a problem hiding this comment.
I know, thing is that I have no idea how to persuade Nextcloud to use the application/vnd.recordare.musicxml+xml MIME type for music xml. It always shows as binary. Any suggestion?
There was a problem hiding this comment.
Absolutely!! There is a dedicated mime file on the server repository. It automatically matches an extension with a mime :)
You need to edit it here. Let me search for an example
There was a problem hiding this comment.
Here :)
There was a problem hiding this comment.
Aaaah nice. That's what I was looking for. Thought the match is done here. Ok I will submit a request there
| <style scoped lang="scss"> | ||
| .sheet-container { | ||
| width: 85vw; | ||
| height: 100vh; |
There was a problem hiding this comment.
100vh is known to cause issues on some mobile devices.
Can you make it reactive instead and make it fit with percentages?
| }) | ||
| } else { | ||
| this.doneLoading(); | ||
| this.OCA.Viewer.close(); |
There was a problem hiding this comment.
Not ok from a code design pov.
The Viewer.vue handles the open/close. Not the views :)
But I guess this will be fixed when the application/octet-stream mime issue is handled
There was a problem hiding this comment.
Yes it is a stupid workaround and I am not happy about it. Best way for me to handle sheet music would be to check both: Mime type and extension.
Internally Opensheetmusicxml checks the first 4 bytes to distinguish if it is XML or MXL (compressed data).
| if (this.isMusicXml) { | ||
| const openSheetMusicDisplay = new OpenSheetMusicDisplay(this.$refs.canvas, { | ||
| autoResize: true, | ||
| darkMode: true |
There was a problem hiding this comment.
Please use the current user settings to decide whether to set dark mode to true or not.
export function isDarkModeEnabled() {
const darkModePreference = window?.matchMedia?.('(prefers-color-scheme: dark)')?.matches
const darkModeSetting = document.body.getAttribute('data-themes')?.includes('dark')
return darkModeSetting || darkModePreference || false
}There was a problem hiding this comment.
I was looking for something like that, where is this method defined?
There was a problem hiding this comment.
I double checked with dark and light theme and the background of the viewer is always black. So true is kinda correct here. Otherwise I could alter the background of the viewer if that makes sense and is doable
There was a problem hiding this comment.
Ah, yes, you're right. I completely forgot!
We do force the dark theme on viewer 😅🙈
Can you then only add a small comment to explain why it's hard coded to true ?
skjnldsv
added
enhancement
|
Hey @bneumann, thanks for the PR! I added some comments :) |
|
I will continue once nextcloud/server#51703 is merged and I can work with the latest version |
|
Hey @bneumann, nextcloud/server#51703 is merged and deployed :) |
|
I took a look today into the latest 32 version of nextcloud and tried to get it to start. I failed miserably and gave up just now. I am usually writing web services in C# which is basically one button to push and everything works. But this PHP thing is driving me nuts. Had the apache running. Wrong user permissions. Fixed the permissions. Installation wants again other user permission. |
2 participants
This is a propsal on how to integrate *.mxl and *.musicxml files in the viewer. It fixes #424
It works fairly well although the MIME type is not correct the sheets I tested. My guess is that Nextcloud symply uses
application/octed-streamfor any unknown binary file. And since the viewer relies on the MIME type I needed to filter on the extnesion as well.Please let me know if there is a better solution. Other then that, here you have it. Sheet music in your nextcloud :)