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

Remove priority key and change delete tag execution logic #350

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

vakssonya
Copy link
Contributor

We are encountering an issue where columns are not being removed correctly using the Delete tag. This occurs because the deletion order depended on the PriorityKey calculation. The higher the PriorityKey, the sooner the column will be deleted. The current PriorityKey calculation algorithm depends on the number of priorities defined for each tag.

var map = Enumerable.Range(byte.MinValue, byte.MaxValue + 1).ToList();
map.RemoveAll(x => this.Any(t => t.PriorityKey == x));
tag.PriorityKey = (byte)map.OrderBy(x => Math.Abs(tag.Priority - x)).First();

For the Delete tag, the Priority is 5, and in the case of PriorityKey calculation for 10 Delete tags in one row sequentially
the PriorityKey value will be - 5 4 6 3 7 2 8 1 9 0. This is incorrect behavior, and this MR presents a solution that sorts tags based on the main priority of the tag. For all tags except delete, the execution order of tags with the same priority is assumed from 1 cell to the last, when for delete tags the execution order from the last cell to the first is assumed.

Comment on lines 70 to 77
var errorsList = new TemplateErrors();
var tagList = new TagsList(errorsList);
foreach (var cell in cells)
{
tagList.Add(CreateInRangeTag<DeleteTag>(range, range.Cell(cell)));
}

tagList.Execute(new ProcessingContext(range, new DataSource(Array.Empty<object>()), new FormulaEvaluator()));
Copy link
Member

Choose a reason for hiding this comment

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

In general, I would recommend not to put "arrange" and "act" in one method - it is harder to grasp the idea of tests this way.

Extract tagList.Execute... into a separate method (e.g. Act(IXLRange range)) and rename the setup methods to womething like AddTagsToRange, AddTagsToWorksheet. Besides, by making cells the last parameters, you can utilize params keyword and make the invocation yet simpler:

AddTagsToRange(rng, "B2");
AddTagsToWorksheet("A3", "A4", "C1", "D1");

Copy link
Member

Choose a reason for hiding this comment

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

Can we prepare another template, maybe with simpler structure but with all different options for placing <<delete>> tag - in the first column, in the first row, and a few in a table range?

Something like this
image
In the expected result, there should be no red cells, easy to visually check.

Copy link
Member

Choose a reason for hiding this comment

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

Also, it would be amalizing if you can cover conditional delete with a test.

Copy link
Contributor Author

@vakssonya vakssonya Apr 10, 2024

Choose a reason for hiding this comment

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

This solution will not fix deleting rows after or inside named range, it requires additional recalculation of the tag cell address. We will try to provide a solution for these cases, but for now the column deletion needs to be fixed

@Pankraty Pankraty added the bug label Apr 11, 2024
@Pankraty Pankraty added this to the 0.2.10 milestone Apr 11, 2024
@Pankraty Pankraty merged commit f21cf97 into ClosedXML:develop Apr 11, 2024
2 checks passed
@Pankraty
Copy link
Member

Awesome job! Thanks for the contribution and keep going ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants