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

updated handling of repeating groupheaders #112

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

Grosskopf
Copy link

@Grosskopf Grosskopf commented Dec 15, 2020

A fix for Bug 108383

Repeating groupheaders were in the page's header while normal groupheaders were in the pages body. So when a normal groupheader was before a repeating groupheader, it would go to body first, write the normal groupheader and then "return" to the next pages header to write the repeating groupheader since that is the next possible header to write to.

In this fix I have changed this, so that a repeating groupheader writes to the page body like the normal groupheader, but also creates a followup pagestyle that has the repeating groupheader in the header.

This was tested mostly in Libreoffice since their bug 51452 is the exact same and the source code is not so different in this area but it was tested once in open office and created similar results.

@leginee
Copy link
Contributor

leginee commented Dec 15, 2020

Awesome! Thanks for the patch.

While reviewing the patch, I noticed that some code has only commented out and not removed. Is there any reason why we want to keep it?

see main/reportbuilder/java/com/sun/star/report/pentaho/output/text/TextRawReportTarget.java (patched line: 1245 to 1248)
1245 /*if (getGroupContext().isGroupWithRepeatingSection()) in to

@Pilot-Pirx Pilot-Pirx requested a review from Mechtilde December 15, 2020 17:01
@Grosskopf
Copy link
Author

Thank you for the quick answer :) True, we don't need that. I tried Squashing it but that didn't quite work the way I expected xD is this ok?

Also, I committed this patch to Libreoffice too (https://gerrit.libreoffice.org/c/core/+/107780/3), so it would be under Apache License for you and under multiple licenses for TDF right? :)

@leginee
Copy link
Contributor

leginee commented Dec 15, 2020

If not needed I would would opt that the dead code is removed. :) Can you remove it? or should I check how I can incorporate this wish?

We need only the Apache License. If I were you I would add the License to the Comments that the patch is under dual License (Apache License, Mozilla License). But this is me.

@Grosskopf
Copy link
Author

If not needed I would would opt that the dead code is removed. :) Can you remove it? or should I check how I can incorporate this wish?

I think I did remove it, is it not gone? this commit history is messy...

We need only the Apache License. If I were you I would add the License to the Comments that the patch is under dual License (Apache License, Mozilla License). But this is me.

should I do this in the code? in the conversations around the patches I mentioned this...

Also I just found bugs I propably did in this patch... I tested another report without repeating groupheaders and it's not working anymore, not sure why... so this shouldn't be merged as of now...

@leginee
Copy link
Contributor

leginee commented Dec 16, 2020

I think I did remove it, is it not gone? this commit history is messy...

Probably I had some caching issue or tomatos on my eyes. I did not see that you removed the code, but now it looks good.

Also I just found bugs I propably did in this patch...

Okay. No issue. We have time.

@Pilot-Pirx Pilot-Pirx changed the title updated handling of repating groupheaders updated handling of repeating groupheaders Jun 18, 2021
@Pilot-Pirx Pilot-Pirx marked this pull request as draft April 27, 2024 22:53
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