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.

Sign up for GitHub

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

Jump to bottom

fix/part-names-key: fix translation of part-names if they are not in … #511

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

fix/part-names-key: fix translation of part-names if they are not in … #511

wants to merge 2 commits into from

Conversation

feuerbart
Copy link
Contributor

Description, Context and related Issue

solve the problem that getParts.xql throws an error if the part names could not be found in lang keys.

Refs #502
(Refs #302 )

How Has This Been Tested?

tested with open-faust A3 (with parts), A2 (without parts)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement

Overview

@feuerbart feuerbart added Type: bugfix A pull request providing a bugfix Area: XQuery labels Dec 18, 2024
@feuerbart feuerbart added this to the 1.0.0 milestone Dec 18, 2024
@feuerbart feuerbart linked an issue Dec 18, 2024 that may be closed by this pull request
[BUG] getParts.xql fails if part-names can't be translated #502
Open
@feuerbart feuerbart self-assigned this Dec 18, 2024
@bwbohl
Copy link
Member

bwbohl commented Dec 19, 2024

I tried to reproduce the error and apparently it does not happen in eXist 6.0.1, but it does in 6.3.0, I don’t know why that is but looking at the code it seems more plausible that the error surfaces.

@bwbohl
Copy link
Member

bwbohl commented Dec 19, 2024

Here are some screenshots:

develop-branch in eXist-db v6.0.1
Screenshot 2024-12-19 at 14 00 06

develop-branch in eXist-db v6.3.0
Screenshot 2024-12-19 at 14 51 49

feuerbart:fix/part-names-key in eXist-db v6.3.0
Screenshot 2024-12-19 at 14 52 19

@bwbohl
Copy link
Member

bwbohl commented Dec 19, 2024

Moreover, I’d like to bring to the attention of @Edirom/edirom-online-developer that the fix provided here is not dpecifically about part labels but concerns all retrievals of langugage variables with eutil:getLanguageString, i.e.:

  • as the function return-type is xs:string it has to return something, which at the moment it does not do if there is corresponding entry in the lang-files
  • This PR introduces a fallback noValueFound for those cases to satisfy the return-type

@bwbohl
Copy link
Member

bwbohl commented Dec 19, 2024

Some thoughts for a more compact query:

…
return
    if($string) then $string else 'noValueFound'

@bwbohl bwbohl self-requested a review December 19, 2024 14:17
@bwbohl
Copy link
Member

bwbohl commented Dec 19, 2024

This is beyond the initial fix idea:

  • If I delete the label of an instrVoice the result will be an empty string in the GUI if there is no corresponding lang-file entry, although a sound label could also be retrieved from the corresponding mei:part.
  • We should come up with a consistent way of how such langVar errors surface in the GUI, e.g., in the XSLTs of the edirom the key gets returned. This is to some degree helpful, because it eases adding the missing keys.

bwbohl
bwbohl previously approved these changes Dec 19, 2024
View reviewed changes
@feuerbart
Copy link
Contributor Author

thanks for investigating @bwbohl !
like the short version.

i just realized that eutil:getLanguageString() does not make use of getLanguageFile.xql (which also loads the edition-specific lang-file) but loads the edirom-lang-file on its own.
but that probably should be another issue for 1.1.0. (including some documentation for custom editiom-lang-files).

@bwbohl
Copy link
Member

bwbohl commented Dec 19, 2024

i just realized that eutil:getLanguageString() does not make use of getLanguageFile.xql (which also loads the edition-specific lang-file) but loads the edirom-lang-file on its own. but that probably should be another issue for 1.1.0. (including some documentation for custom editiom-lang-files).

Good catch!

bwbohl
bwbohl reviewed Dec 19, 2024
View reviewed changes
add/data/xqm/eutil.xqm Outdated
Comment on lines 269 to 275
let $string :=
if($file//entry[@key = $key])
then (
$file//entry[@key = $key]/string(@value)
) else (
'noValueFound'
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let $string :=
if($file//entry[@key = $key])
then (
$file//entry[@key = $key]/string(@value)
) else (
'noValueFound'
)
let $projectFile := doc(edition:getLanguageFileURI($edition, $lang))
let $projectString := $projectFile//entry[@key = $key]
let $ediromString := $file//entry[@key = $key]
let $string := if($projectString) then $projectString else $ediromString

Copy link
Member

Choose a reason for hiding this comment

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

combined with the suggested return statement this could work! But I haven’t tested it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if i add
let $edition := request:get-parameter('edition', '')
it kind of does, but i guess not as intended: it will print the right values but it looks like the edition lang-file won't be found. probably i'm missing something?

should we ignore the custom edition lang-file for now (aka just use the suggested return statement) and come back to this for 1.1.0?

@feuerbart
Copy link
Contributor Author

feuerbart commented Dec 20, 2024 •
edited
Loading

having a look at the baudi dataset (sources/music/cantatas/baudi-01-8a00c113.xml) i realized it probably might be best to not use the label-attr. to translate via the lang-file but the codedval-attr.
baudi uses codedval-attr. and not label-attr., e.g.:
<perfRes xml:id="part_06" codedval="flute.i"/>
but probably use codes like MEI guidelines suggest: https://music-encoding.org/guidelines/v5/content/metadata.html#headerWorkInstrumentation (-> listing 85)

so the 'long solution' could probably be:

  • add standardized codes to edirom lang-file
    e.g. <entry key="perfMedium.sa" value="Violin"/>
    and probably sa.i for Violin I

if there is a label-attr. for the part -> use label-attr.
if there is a codedcal-attr. for the part:

  • load edirom & edition lang-file
    -> resolve codedval-attr.

i think that could be a good way also considering the MEI-guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwbohl bwbohl bwbohl left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@feuerbart feuerbart

Labels
Area: XQuery Type: bugfix A pull request providing a bugfix
Projects
Edirom Development
Status: Approved
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

[BUG] getParts.xql fails if part-names can't be translated
2 participants
@feuerbart @bwbohl