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

Enhancement to add excel formula options (#1643) #1644

Merged

Conversation

speckyspooky
Copy link
Contributor

Enhancement to add excel formula options (#1643)

@speckyspooky speckyspooky added this to the 4.16 milestone Apr 23, 2024
@speckyspooky speckyspooky added the Enhancement Small change to improve the current supported functionality label Apr 23, 2024
@speckyspooky speckyspooky self-assigned this Apr 23, 2024
@speckyspooky
Copy link
Contributor Author

I researched the original changes of JT and compared it with the latest version of our master-branch.
The change is no copy or direct merge of the change. It is focused on the formula-scope.
So it is a new version at all but it can be compared with the original changes of JT.

@@ -786,11 +787,12 @@ protected int getRichTextRunIndexForStart(List<RichTextRun> richTextRuns, int st
* @param defaultFont The font to be used prior to the first RichTextRun.
* @param widthMM The width of the output.
* @param richTextRuns The list of RichTextRuns to be applied to the string
* @return The heigh, in points, of a box big enough to contain the formatted
* @param wrap The text use wrapped style
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment for wrap is not clear to me (and grammar errors)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it.

Copy link
Contributor

@hvbtup hvbtup left a comment

Choose a reason for hiding this comment

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

There's too much code changed to understand it.
Is all of this actually related to formulas?

I mean, you added a whole line-breaking algorithm.
From what I did decades ago when I developed the wordaxe hyphenation library including using it in a new a paragraph line-breaking algorithm for the Python ReportLab library, I know that those line-breaking algorithms are very fragile kind of program, where things can easily go wrong.

Is it possible for you to split the formula support and the layout enhancements you mentioned (which exactly?) into two separate PRs?

@speckyspooky
Copy link
Contributor Author

speckyspooky commented Apr 24, 2024

Of course there are some changes because the biggest change is to add the "formula"-options and give the formula-values the option for the correct styling on excel side.

You talking about the "new a paragraph line-breaking algorithm" - which section do you mean exactly, do you mean the else-block with the "TextMeasurer measurer"?
Then I will check if it make sense to split it from my side or may be we can use an additional user-property for the "new line algroithm" (default: false).

@hvbtup
Copy link
Contributor

hvbtup commented Apr 24, 2024

I mean the height calculation in calculateTextHeightPoints.

@speckyspooky
Copy link
Contributor Author

I know and therefore my specific question for this method because, calculateTextHeightPoints-Method has know 2 options to calculate the height based on:

  • LineBreakMeasurer measurer (old option)
  • TextMeasurer measurer (new option)

The options will be controled via "wrap"-variable which is comming from "cell.getCellStyle().getWrapText()".
This method is given thorugh the apach poi library to definethe wraped style of a cell - I attached the documentation below.

What is there the problem for you that the height "won't be calculated correctly" or that we get "exceptions" (which is currently catched)?

What do you think about an additional uer-property - would it be helpful to handle it?
(No problem to add them.)

I would make an additional comment on the title & ticket put we can leave it in the same PR.
If we would use a classic source-code-merge of the JT-changes we had more nontransparencies.

getWrapText()

@hvbtup
Copy link
Contributor

hvbtup commented Apr 24, 2024

Thomas, if you say this is needed then I believe you :-).
It's just that I don't understand what's the height calculation got to do with formulas...
My naive feeling is that when a cell contains a formula, then it's not possible to automatically compute a meaningful height anyway, because we don't know the value (only the formula).

I'm +0.5 for merging. Even if I don't understand all the changes, probably they will have an effect only for cells with formulas, which are a new feature. Thus I think that the risk of breaking existing reports is near zero.

@speckyspooky speckyspooky merged commit 8e2041b into eclipse-birt:master Apr 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Small change to improve the current supported functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants