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

Adjust metadata replacement #5800

Merged
merged 4 commits into from
Dec 4, 2023
Merged

Conversation

henning-gerhardt
Copy link
Collaborator

fixes #5799

@solth solth requested a review from matthias-ronge October 6, 2023 11:46
@henning-gerhardt henning-gerhardt changed the title Adjust metada replacement Adjust metadata replacement Oct 10, 2023
@matthias-ronge
Copy link
Collaborator

This fix is a functional change. Previously, with $meta.x variables only the first child was checked, and if this is empty, the variable is resolved on the top structure.

With this change, the first grandchild is now also taken into account, which would result in a different output value in other constellations:

📄 topstruct
  ├─ 📝 md: Foo
  └─ 📄 firstchild
      └─ 📄 firstgrandchild
          └─ 📝 md: Bar

Before: $(meta.md) → “Foo” (firstchild has no ‘md’, so resolve from topstruct)
After: $(meta.md) → “Bar” (firstchild has no ‘md’, found on firstgrandchild, value from topstruct ignored)

Maybe fix like this:

       if (!allChildren.isEmpty()) {
           allFirstchildValue = MetadataEditor.getMetadataValue(allChildren.get(0), variableFinder.group(5));
/*»*/      if (Objects.isNull(allFirstchildValue)) {                                                     
/*»*/          allFirstchildValue = determineReplacementForTopstruct(variableFinder, dollarSignIfToKeep);
/*»*/      }
           if (Objects.isNull(allFirstchildValue)) {
               List<LogicalDivision> firstChildChildren = allChildren.get(0).getChildren();
               if (!firstChildChildren.isEmpty()) {
                   allFirstchildValue = MetadataEditor.getMetadataValue(firstChildChildren.get(0), variableFinder.group(5));
               }
           }
       }

This should maintain the original behaviour, and fix your problem as well.

Copy link
Collaborator

@matthias-ronge matthias-ronge left a comment

Choose a reason for hiding this comment

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

See comment above

@henning-gerhardt
Copy link
Collaborator Author

My intention was to look first into the first child and then in the first grandchild and if both locations are not returning an result look into the top element.

But your code change suggestion is not working without an other needed change: determineReplacementForTopstruct method is returning an empty string if in the top element not result is found (hidden by the dollarSignIfToKeep variable which is used as return value in not found case and the dollarSignIfToKeep variable is an empty string in many cases) so the following check condition must be extended to check for an empty string to get the right meta data.

@solth solth requested a review from matthias-ronge November 28, 2023 14:56
Copy link
Collaborator

@matthias-ronge matthias-ronge left a comment

Choose a reason for hiding this comment

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

Just one more little thing.

@solth solth merged commit b8aec9c into kitodo:master Dec 4, 2023
2 checks passed
@henning-gerhardt henning-gerhardt deleted the adjust_metada_replacement branch December 4, 2023 13:44
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.

Meta data of newspaper issues can not be accessed on variable replacement
3 participants