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

[wip] Sort children of OrderedGroup etc. by @index #478

Merged
merged 6 commits into from
May 13, 2020

Conversation

kba
Copy link
Member

@kba kba commented May 12, 2020

This overrides the exportChildren of the generated OrderedGroupType and sorts the contained RegionRefIndexed (and OrderedGroupIndexed and UnorderedGroupIndexed) by their resp. @index` attribute.

While this works as expected, the output still is unsorted and I am temporarily stumped as to why.

While I'm confidant that it should not involve much more code, I currently cannot see what the issue ist. Will try again tomorrow but if someone has an idea, I am happy hearing them.

Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

Something really bad happens to my comments/suggestions here. Either they get duplicated many times, or lost!

Anyway, I think there has been a misunderstanding...

tests/model/TEMP1_Gutachten2-2.xml Outdated Show resolved Hide resolved
tests/model/TEMP1_Gutachten2-2.xml Outdated Show resolved Hide resolved
tests/model/test_ocrd_page.py Outdated Show resolved Hide resolved
ocrd_models/ocrd_models/ocrd_page_generateds.py Outdated Show resolved Hide resolved
ocrd_models/ocrd_page_user_methods.py Outdated Show resolved Hide resolved
kba and others added 4 commits May 13, 2020 12:35
* `get_AllIndexed()`: list all RegionRefIndexed, OrderedGroupIdexed and
  UnorderedGroupIndexed elements, sorted ascending by their index
* `clear_Allindexed()`: Removes all RegionRefIndexed,
  OrderedGroupIdexed and UnorderedGroupIndexed elements and reeutrns
  them
* `add_AllIndexed(elmenets): Add a variety of RegionRefIndexed, OrderedGroupIdexed and UnorderedGroupIndexed elmenets with proper sort byt index

On export of an OrderedGroup, the elements are listed with
`get_AllIndexed` and hence should have the correct order. Also when
exporting: all empty OrderedGroupIdexed/UnorderedGroupIndexed will be
replaced with `RegionRefIndexed` with appropriate `index` nd `regionRef`
@bertsky bertsky self-requested a review May 13, 2020 15:45
Copy link
Collaborator

@bertsky bertsky left a comment

Choose a reason for hiding this comment

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

This looks fantastic – thanks a lot!

@kba kba merged commit 42c1672 into OCR-D:master May 13, 2020
@kba kba deleted the readingorder-exportchildren branch May 13, 2020 18:03
@bertsky
Copy link
Collaborator

bertsky commented May 13, 2020

Alas, the new export functionality only arrived at OrderedGroupIndexedType, but not OrderedGroupType in the generated code. It's strange, because your user methods all have a disjunction for both types. Maybe generateDS ignores the disjunction? Or is the committed ocrd_page_generateds.py from a prior version which did not yet have that disjunction for both types?

@kba
Copy link
Member Author

kba commented May 14, 2020

Obviously an error in the regex for class_names, fixing.

@kba
Copy link
Member Author

kba commented May 14, 2020

Cannot reproduce the issue, I see defintion for get_AllIndexed, exportChildren etc. for both OrderedGroupType and OrderedGroupIndexedType:

https://github.com/OCR-D/core/pull/478/files#diff-4e889da1f943107585e46a514c8513b3R5351-R5369

and

https://github.com/OCR-D/core/pull/478/files#diff-4e889da1f943107585e46a514c8513b3R6140-R6158

Not sure what I can do. Can you provide a small code sample to test?

@bertsky
Copy link
Collaborator

bertsky commented May 14, 2020

You are right. Don't know just what I saw. Everything seems to be working fine. Sorry for the noise!

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.

2 participants