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

Refactor comments #1227

Open
wants to merge 8 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@igitur
Copy link
Member

commented Jun 4, 2019

Based on #1204 but with some additional refactoring.

I'm still not sure whether we should hard-code the 7.5. I'm not even sure what this value implies. It must again be some assumption about the average width of a character.

Fixes #1157
Fixes #1203

@igitur

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

Damn, the comments are way more complicated than I thought. Especially the VML part.

Sizing of the comments is still not ideal. Size is stored both in the shape's style attribute as well as the anchor element. Sometimes they conflict (if you work it out in exact pixels). For this reason, the AddingComments test leads to 2 not identical files (zAddingComments.xlsx vs AddingComments.xlsx). But the visual effect is negligible.

@igitur igitur added this to the v0.95 milestone Jun 4, 2019

// find cell by reference
var cell = ws.Cell(c.Reference);

var xlComment = cell.Comment;
var shapeIdString = shape?.Attribute("id")?.Value;

This comment has been minimized.

Copy link
@igitur

igitur Jun 5, 2019

Author Member

I don't have any examples of where this attribute is missing, but I'm trying to cover that case.


// **** MAYBE FUTURE SHAPE SIZE SUPPORT
XDocument xdoc = GetCommentVmlFile(worksheetPart);
var shapes = GetCommentShapes(worksheetPart);

This comment has been minimized.

Copy link
@igitur

igitur Jun 5, 2019

Author Member

All shapes from the VML file should be parsed and returned only once.


shape.Remove();

This comment has been minimized.

Copy link
@igitur

igitur Jun 5, 2019

Author Member

Not sure what the goal of this was.

@@ -1184,39 +1194,37 @@ private static XLMarker LoadMarker(IXLWorksheet ws, Xdr.MarkerType marker)

#region Comment Helpers

private XDocument GetCommentVmlFile(WorksheetPart wsPart)

This comment has been minimized.

Copy link
@igitur

igitur Jun 5, 2019

Author Member

These 2 methods merged into 1.

XDocument xdoc = null;

foreach (var vmlPart in wsPart.VmlDrawingParts)
// Cannot get this to return Vml.Shape elements

This comment has been minimized.

Copy link
@igitur

igitur Jun 5, 2019

Author Member

It seems that because the VML file starts with an xml element, it can't be parsed with standard OpenXml, even though the inner elements are valid OpenXml elements, (e.g. ShapeType and Shape). I've even tried writing xml's inner XML elements to a memory stream and using OpenXMLReader to parse that, but it still doesn't work. Maybe due to missing namespace declarations. It's a pity that we have to resort to returning XElements.

@@ -355,51 +368,45 @@ private void CreateParts(SpreadsheetDocument document, SaveOptions options)
this.ResumeEvents();
}

private void DeleteComments(WorksheetPart worksheetPart, XLWorksheet worksheet, SaveContext context)
private bool DeleteExistingComments(WorksheetPart worksheetPart, XLWorksheet worksheet, WorksheetCommentsPart commentsPart, VmlDrawingPart vmlDrawingPart)

This comment has been minimized.

Copy link
@igitur

igitur Jun 5, 2019

Author Member

Avoids having to create a new VmlDrawingPart.

: base(defaultFont)
{
Initialize(cell, shapeId);
}

This comment has been minimized.

Copy link
@Pankraty

Pankraty Jun 5, 2019

Member

I would move from having 6 constructors toward 2-3 with optional parameters.

How about this?

public XLComment(XLCell cell, IXLFontBase defaultFont = XLFont.DefaultCommentFont, int? shapeId = null)
: base(defaultFont ?? XLFont.DefaultCommentFont)
{
  if (cell == null) throw new ArgumentNullException(nameof(cell));
  shapeId = shapeId ?? cell.Worksheet.Workbook.ShapeIdManager.GetNext();
  Initialize(cell, shapeId.Value);
}

Ideally, only one constructor should call base(), all the others call this() with different sets of parameters. But this is not the case as the base class already has different constructors.

This comment has been minimized.

Copy link
@igitur

igitur Jun 5, 2019

Author Member

Ok, good point.

This comment has been minimized.

Copy link
@igitur

igitur Jun 10, 2019

Author Member

This has been fixed.

@igitur igitur force-pushed the igitur:refactor-comments branch from bf107c1 to acd51e4 Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.