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 17 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

@krHERO
Copy link
Member

krHERO commented Jun 5, 2024

ping @nikobeer

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.

please fix multiRest handling

@bwbohl
Copy link
Member

bwbohl commented Jun 20, 2024

pinging @riedde

@riedde
Copy link
Contributor Author

riedde commented Jun 21, 2024

@bwbohl I reviewed the function measure:getMeasureLabel(). It seems that there was an else case missing (added at 14837c2). I'm not sure if this coveres all cases but it is much more better than before and works with several datasets. I can offer you my dataset for testing if you like (three editions).

@riedde
Copy link
Contributor Author

riedde commented Sep 18, 2024

While testing I noticed that there is a bug when loading the measure based view with parts. I have to investigate this.

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

Successfully merging this pull request may close these issues.

3 participants