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

Fix that col width is not written if other col props are set #73

Merged
merged 1 commit into from
Jul 17, 2023
Merged

Fix that col width is not written if other col props are set #73

merged 1 commit into from
Jul 17, 2023

Conversation

LoadingByte
Copy link
Contributor

Thank you a lot for immediately merging all three pull requests! I really appreciate it :)

I'm terribly sorry that you already published a new release, but there was still a long-standing bug in there which I didn't notice until just now: when properties other than the width are set on a ColumnStyle, the width is no longer written to the ODS. That bug is fixed by this changeset.

For example, when hiding a column, its width was no longer written to
the ODS file.
@miachm
Copy link
Owner

miachm commented Jul 17, 2023

Hi LoadingByte!

It seems right it's only being used width. But I am wondering what will happend when we implement other column styles?

@LoadingByte
Copy link
Contributor Author

In that case, we'd need to introduce a key object in OdsWritter, which only contains the sub-properties of ColumnStyle that are actually written to a <style:style> element.

As an alternative, we could externalize these sub-properties to a ColumnStyleOds class, which is embedded into and delegated to by ColumnStyle, e.g.:

class ColumnStyleOds {
    Double width;
}

class ColumnStyle {
    private ColumnStyleOds columnStyleOds = ColumnStyleOds();
    public Double getWidth() { return columnStyleOds.width; }
    public void setWidth(Double width) { columnStyleOds.width = width; }
}

The class ColumnStyleOds would then function as the key object in OdsWritter.

@miachm
Copy link
Owner

miachm commented Jul 17, 2023

Giving a second thought, maybe we should wait until that event happens. We can not figure the right solution until we find the problem.

@miachm miachm merged commit b21e221 into miachm:master Jul 17, 2023
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