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

Feature: Auto measure label handling #302

Open
wants to merge 61 commits into
base: develop
Choose a base branch
from

Conversation

riedde
Copy link
Contributor

@riedde riedde commented Dec 24, 2022

Better handling of Measure labels

  • joining labels when multiple measures pointing to same zone (avoids labels like 1/9)
  • also works with already joined labels (multiRests)
  • introducing diacritical labels (supplied and del)

needs:

  • code clean up
  • introducing functions (avoid redundancy)

@bwbohl
Copy link
Member

bwbohl commented Feb 5, 2024

@riedde what’s the status of this draft, any chance to resolve conflicts and close TODOs in the near future?

@bwbohl bwbohl requested a review from nikobeer February 5, 2024 22:52
# Conflicts:
#	add/data/xql/getMeasures.xql
@riedde
Copy link
Contributor Author

riedde commented Feb 7, 2024

@bwbohl conflicts resolved. I'll have a look at the TODOs

@riedde riedde marked this pull request as ready for review February 7, 2024 07:25
@riedde riedde requested a review from bwbohl February 7, 2024 07:25
@bwbohl
Copy link
Member

bwbohl commented Feb 13, 2024

What’s an appropriate dataset for testing this?

@riedde
Copy link
Contributor Author

riedde commented Feb 14, 2024

@bwbohl
Copy link
Member

bwbohl commented Feb 26, 2024

Tested with the klarinettenquintett dataset:

  • label seem correct but Concordance navigation fails ,e.g. On Baermann D+-st with the multirest in Allegro/Klarinette M.1–11 ff.

@bwbohl
Copy link
Member

bwbohl commented Feb 26, 2024

The multiRest at the start of the Allegro in D+-st is also missing "–11"

Copy link
Member

@bwbohl bwbohl left a comment

Choose a reason for hiding this comment

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

multiRest handling doesn’t seem to work properly with data-set klarinettenquintett

:
: @param $mei the mei file
: @param $mdivID the ID of the mdiv
: @return Maps (as strings)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can! In fact, the file https://github.com/Edirom/Edirom-Online/blob/develop/add/data/xql/getMeasuresOnPage.xql in current develop branch already outputs proper JSON. The change here is a step backwards.

(: TODO: überlegen, wie die Staff-spezifischen Ausschnitte angezeigt werden sollen :)
local:getMeasures($mei, $surface)
}
return (
Copy link
Member

Choose a reason for hiding this comment

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

please keep the array constructor (or force measure:getMeasuresOnPage#2 to return an array)

Copy link
Contributor Author

@riedde riedde Dec 16, 2024

Choose a reason for hiding this comment

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

At 2ad0a2d
The round brackets are removed. The function returns a map. Why should there be an array?

Copy link
Member

Choose a reason for hiding this comment

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

measure:getMeasures#2 returns a sequence of maps. If the sequence is gt 1 this will automatically be serialized to a JSON array. If the sequence is 1 the return type will be a JSON object. If it's 0 the return type will be NULL.
This is quite annoying for the frontend so it's much nicer to always return an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterstadler Done at 01568d2 I don't have time today to work on this, so if necessary, please fix directly before tomorrow's release.

add/data/xql/getMeasures.xql Outdated Show resolved Hide resolved
add/data/xqm/measure.xqm Outdated Show resolved Hide resolved
add/data/xqm/measure.xqm Outdated Show resolved Hide resolved
add/data/xqm/measure.xqm Outdated Show resolved Hide resolved
add/data/xqm/measure.xqm Outdated Show resolved Hide resolved
testing/XQSuite/measure-tests.xqm Outdated Show resolved Hide resolved
@peterstadler
Copy link
Member

peterstadler commented Dec 10, 2024

There are still issues with MEI multiRest.
Consider the XQuery

xquery version "3.1";

declare namespace mei="http://www.music-encoding.org/ns/mei";

import module namespace measure = "http://www.edirom.de/xquery/measure" at "xmldb:exist:///db/apps/Edirom-Online/data/xqm/measure.xqm";


doc("/db/apps/Edirom-Online/testing/XQSuite/testdata/multiMeasureRests.xml")/mei:mdiv//mei:measure[1] 
    => measure:getMeasureLabel()

which will properly output

<xhtml:span xmlns:xhtml="http://www.w3.org/1999/xhtml">
    <xhtml:span>107</xhtml:span>
</xhtml:span>

but will fail for mei:measure[2] (which features multiRests):

<measure xml:id="x9cafb006-025b-4019-9430-936ab85a62db" right="rptend" n="108">
<staff xml:id="x734d85be-0016-46cb-870f-c83832a5a9db" n="1">
<layer xml:id="x329eefc9-112e-45be-8d11-6192be6b9eba" n="1">
<multiRest xml:id="xe9ace48e-8c8f-4e09-b148-669c9c622787" num="2"/>
</layer>
</staff>
<staff xml:id="x103f0765-102b-45ba-935b-92991d7eb77f" n="2">
<layer xml:id="x6e1af6eb-b22a-47e5-b62a-c77da263eb7d" n="1">
<multiRest xml:id="x5b945b6f-ae28-44c3-b972-cd7e82453eda" num="2"/>
</layer>
</staff>

UPDATE: I just added a test for measure:getMeasureLabel#1 in 4d745b9

@peterstadler
Copy link
Member

I tried to update the code but there's still a test failing for measure:getMeasureLabel#1 at

<xhtml:span>{$measureLabel || '–' || (number($measureLabel) + number($measure//mei:multiRest/@num) - 1)}</xhtml:span>

because there can be multiple <mei:multiRest> (in multiple layers) in an <mei:measure>.

@krHERO krHERO modified the milestones: 1.0.0, 1.2.0 Dec 18, 2024
@krHERO
Copy link
Member

krHERO commented Dec 18, 2024

at the community meeting 2024-12-18 we agreed to move this PR to next release.
but extract parts from it and solve with #302 for v1.0.0 @feuerbart

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

5 participants