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

Enhance the excel output to get the formatting option of text-indent (#1666) #1667

Merged
merged 3 commits into from
May 7, 2024

Conversation

speckyspooky
Copy link
Contributor

Enhance the excel output to get the formatting option of text-indent (#1666)

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

I have added the test results, screens and original test reports at the issue-ticket.

The idea is to sum the margin & padding and calculate it to the indent-factor.
The text-indent is a special MS Excel unit and I researched an according translation-factor to get the indent-values.

To sum the margin & padding the values must be synced to the same value. I decieded to use "mm" as default unit,
because it was used within Spudsoft as default-unit in different situations.

The main changes are given at the CellContentHandler within the new method "overlayBirtCellStyleSpacing()"and at the StyleManager "createStyle()".
The method "overlayBirtCellStyleSpacing()" handle the whole topic of the correct style creation of the birtCellStyle
and the "createStyle()" includes the new calculation of the text-indent for excel.

Hint:
One of the main topics is that "overlayBirtCellStyleSpacing()" solve some topics with the style properties.
Because the text element is handled in a different way for excel because it is not like "Word" or "PDF" an own element with a "p"(aragraph), it is a combination of cell- and text-style.
Therefore the method handle the overlay to set the correct margin and padding for the excel-cell.

Copy link
Contributor

@wimjongman wimjongman left a comment

Choose a reason for hiding this comment

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

LGTM

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.

I see the same typo at least two times in README.md: "teh" instead of "the".

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.

I'm not sure if the default value of True for ExcelEmitter.DisplayTextIndent is a good idea.
It will probably change (aka break) the layout of existing reports.
IMHO the default should be to not use indentation.

Furthermore, I'm not sure if using the margin of the element inside a cell is a good idea.
It is hard to predict for a developer what should happen if there are more than one elements inside a cell.
Instead, only the padding of the cell should be used IMHO.

@speckyspooky
Copy link
Contributor Author

I will remove the typo and I will double check the behavior of multiple content.

But from my side it would be good to have the indention active because I would expect the indent by default.
It is for me the same like PDF or Word. There the indent is working directly without any activation. Yes, I know this is a change of the behavior therefore the user property to switch of the change.

I will check the padding of the cell but I will have also an indent if the cell has "no padding" but the labels / text-elements would have a margin & padding. Then I wants to get an indent.
But for that I will add an additional user-property for "ExcelEmitter.DisplayIndentMode" that we can configure the calculation with the options "SpacingCell", "SpacingElement" and "SpacingAll".

@hvbtup
Copy link
Contributor

hvbtup commented May 6, 2024

This may be a silly question, but:
Can the user-properties be defined at the top-level (i.e. the report itself) and will they inherit from the corresponding container?
If yes, I'm fine with "indentation is the default".

What should happen if the layout is more complex, e.g. a Grid Item or Table Item inside a Cell, possibly at 3 or 4 levels?
Such reports do exist (called sub-reports in some other tools).
In fact in our case, they are quite common, however we don't use them with Excel output.

Personally, I don't think it is possible to correctly calculate the spacing in such more general cases.
But I expect users to expect this :-).
Thus I think it should be clarified to what extent the indentation can be expected to work correctly.

@speckyspooky
Copy link
Contributor Author

The indent will be calculated only on label / text-element level and will take a look to the parent cell.
The nested tables are handled with the default behavior and so the structures will be restructured from your mentioned case to a possible excel table/grid.
This situation of multiple table-in-table-in-table-in-grid will be handled before and the change will work afterwards if the cell contains a label/text-element.

And in such special cases the ExcelEmitter has a default behavior because he doesn't allow to nest to many tables in a cell.
Then you will get an error by default.

@speckyspooky speckyspooky requested a review from hvbtup May 6, 2024 20:18
@speckyspooky
Copy link
Contributor Author

question 1: Can the user-property defined at top-level / report-level?
answer:

  • Yes, you can configure the user-property on report level and no indent will be set.
  • But you can use also mixed usage if you have different tables & grids then you can set it on the tables which souldn't have an indent.
  • The lowest level is the label/text-lement-level him self.

question 2: What will be happend if 2 (more) elements will be added in a cell and both element have a different margin & padding?
answer:

  • The spacing values of the first cell element will used for all text-elements on a cell. It is not possible to have different indents at a cell.

question 3: What will be happend if a cell contains further inner tables/grids?
answer:

  • The excel emitter resolves the internal grids directly and the last action will be the cell-content formating. This will be done based on the first cell-conetnt-element. The indent of the first element will be used.

Added user property for indent mode
I have also added the according user property to control the indent-calculation with the options "SpacingCell", "SpacingElement" and "SpacingAll".

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.

The text in ..../uk/co/spudsoft/birt/emitters/excel/README.md could use some polish, but this can be done later, and should preferably be done by a native English speaker.

@speckyspooky speckyspooky merged commit 71ce2d9 into eclipse-birt:master May 7, 2024
3 checks passed
@wimjongman
Copy link
Contributor

Thanks, Thomas!

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.

3 participants