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

Added Page Breaks #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

thedude61636
Copy link

I've added page breaks

public XlsxWriter AddPageBreak(); //adds a page break at the current index and column
public XlsxWriter AddRowPageBreak(); //adds a horizontal page break at the current row
public XlsxWriter AddColumnPageBreak(); //adds a vertical page break at the current column

@salvois
Copy link
Owner

salvois commented Nov 24, 2024

Hi @thedude61636 ,
thanks for your contribution! I hope I can have a look in the weekend.

README.md Show resolved Hide resolved
@@ -66,6 +66,7 @@ public static void Run()
.BeginRow().Write("Underline").Write().Write("Double", XlsxStyle.Default.With(XlsxFont.Default.WithUnderline(XlsxFont.Underline.Double)))
.BeginRow().Write("Underline").Write().Write("SingleAccounting", XlsxStyle.Default.With(XlsxFont.Default.WithUnderline(XlsxFont.Underline.SingleAccounting)))
.BeginRow().Write("Underline").Write().Write("DoubleAccounting", XlsxStyle.Default.With(XlsxFont.Default.WithUnderline(XlsxFont.Underline.DoubleAccounting)))
.AddPageBreak()
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe adding a separate, minimal example to demonstrate page breaks could be useful, because I feel like the "simple" example was already no longer so simple. Or maybe we can use the header/footer to group printing-related functionality.

src/LargeXlsx/Worksheet.cs Show resolved Hide resolved
_streamWriter.Write($"<rowBreaks count=\"{_rowBreaks.Count}\" manualBreakCount=\"{_rowBreaks.Count}\">");
foreach (var rowNum in _rowBreaks.OrderBy(r => r))
{
_streamWriter.Write($"<brk id=\"{rowNum}\" max=\"16383\" man=\"1\"/>");
Copy link
Owner

Choose a reason for hiding this comment

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

We already have a constant for this in Util.MaxColumnCount. I feel like we should use it rather than hardcoding the magic number, maybe moving the constant from Util to this file.

Copy link
Author

Choose a reason for hiding this comment

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

alright but max column count is private in that class we either make it public or move it to this class or we can make a constants class and put the max row and column count there , it's kinda weird having the max row count in the worksheet and the max column count in util

Copy link
Owner

Choose a reason for hiding this comment

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

I agree it feels strange having them separate. This is why I proposed having both in the Worksheet class, if it makes sense to you. Likely they ended into different places because they were born at different times for different needs, and I was not careful enough to notice, sorry.

_streamWriter.Write($"<colBreaks count=\"{_columnBreaks.Count}\" manualBreakCount=\"{_columnBreaks.Count}\">");
foreach (var colNum in _columnBreaks.OrderBy(c => c))
{
_streamWriter.Write($"<brk id=\"{colNum}\" max=\"1048575\" man=\"1\"/>");
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above for the MaxRowNumbers constant (which we may rename to MaxRowCount).

@@ -257,6 +262,30 @@ public void AddMergedCell(int fromRow, int fromColumn, int rowCount, int columnC
var toColumnName = Util.GetColumnName(fromColumn + columnCount - 1);
_mergedCellRefs.Add($"{fromColumnName}{fromRow}:{toColumnName}{toRow}");
}

public void AddRowPageBreak()
Copy link
Owner

Choose a reason for hiding this comment

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

In my understanding adding page breaks is an operation you could do at any time, because they are accumulated in RAM, just like merged cells or autofilter.
For consistency, I feel like AddRowPageBreak and AddColumnPageBreak should accept the row and column number (1-based for consistency too).

_currentWorksheet.AddPageBreak();
return this;
}
public XlsxWriter AddRowPageBreak()
Copy link
Owner

Choose a reason for hiding this comment

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

XlsxWriter could expose two overloaded versions of AddRowPageBreak and AddColumnPageBreak: one accepting the 1-based row/column number, and one using the current insertion point.

@@ -349,6 +349,24 @@ public XlsxWriter AddMergedCell(int fromRow, int fromColumn, int rowCount, int c
public XlsxWriter AddMergedCell(int rowCount, int columnCount)
{
return AddMergedCell(CurrentRowNumber, CurrentColumnNumber, rowCount, columnCount);
}
public XlsxWriter AddPageBreak()
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like AddPageBreak, adding the two breaks at once, may cause confusion, or at least it would be not obvious to me: in a typical use case I guess I'd setup column breaks when writing my header, and row breaks when streaming my content.

Also, minimal issue and sorry for nitpicking, please be consistent with spacing and indentation of the surrounding code.

Copy link
Author

Choose a reason for hiding this comment

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

you're right it does sound confusing
i can remove all the methods and make two methods (one for horizontal and one for vertical) that takes a default value if it's null it inserts at insertion point and if not, it inserts at specified value

and that's alright nitpick all you want :D

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