Markdown improvements regarding tables, nested statements and special character support in links #1647
Conversation
weissm
changed the title
|
I will take a look at this. Just need a little time. Thanks! |
|
@weissm Using your Nested Tables example page, this results in markdown as follows, with a table header divider line after the table, not before: | **🛈** | **Test Protocol** Markdown generation of nested tables <ol><li>Here is a nested table</li><li>Export this to an Markdown file or Copy as Markdown</li><li>Confirm that a netsted table is generate</li></ol> | | |
| :--- | :--- | :--- | :--- |and when converted back to OneNote content, looks like this: 
The divider line should be removed, or before the table and translated to an actual divider |
|
|
||
| public PrefixClass(string set_indents = "", string set_tags = "", string set_bullets = "", string tablelistid = "") |
There was a problem hiding this comment.
Remove the set_ prefix on these parameter names.
Use a default constructor to take advantage of the field default values above, otherwise the default parameter values are redundant here.
There was a problem hiding this comment.
set_ removed. The default parameters I left equal to allow instantiation with detault parameters.
| break; | ||
|
|
||
| case "T": | ||
| { | ||
| var context = DetectQuickStyle(element); | ||
| if (context is not null) | ||
| if (contained) // not in table cell |
There was a problem hiding this comment.
You've lost calls to DetectQuickStyle and Stylize here; OneNote can add quickStyleIndex to T, OE, OEChildren - one or any of them.
There was a problem hiding this comment.
Added DetectQuickStyle again. Let it out as I did not see this actually been used in OneNote so far.
| } | ||
|
|
||
| if (index >= 0) | ||
| if (element.GetAttributeValue("quickStyleIndex", out int index)) |
There was a problem hiding this comment.
The code as-is, only detects quick style on OE. It's missing detection on OEChildren and T. That why the deleted code look at parent elements until it found a quickStyleIndex. Text may not be styled correctly.
There was a problem hiding this comment.
The intention here was to have the check only in the current level for both OE and T as in the intially code. The recursive nature did not work here and it should not be needed, as it is handled at each stage already.
|
|
||
| switch (symbol) | ||
| { | ||
| case 3: // to do | ||
| case 8: // client request | ||
| case 12: // schedule/callback | ||
| case 28: // to do prio 1 | ||
| case 71: // to do prio 2 | ||
| case 28: // todo prio 1 |
There was a problem hiding this comment.
OneNote calls them "To do" tags, not "Todo" tags
| case 64: retValue = (":star: "); break; // custom 1 | ||
| case 70: retValue = (":one: "); break; // one | ||
| case 116: retValue = (":busts_in_silhouette: "); break; // busts_in_silhouette | ||
| case 117: retValue = (":notebook: "); break; // notebook |
| case 70: retValue = (":one: "); break; // one | ||
| case 116: retValue = (":busts_in_silhouette: "); break; // busts_in_silhouette | ||
| case 117: retValue = (":notebook: "); break; // notebook | ||
| case 118: retValue = (":mailbox: "); break; // contact |
There was a problem hiding this comment.
118 is a letter, not a mailbox
| while (nestedtables.Count() != 0) | ||
| { | ||
| var nestedtable = nestedtables.First(); | ||
| writer.WriteLine(prefix.indents + "<details id=\"nested-table" + nestedtable.index + "\" open>"); |
There was a problem hiding this comment.
Shouldn't <details> have a closure </details> ?
There was a problem hiding this comment.
good catch. Added closing and additional
pair to allow collapse tables.
| while (nestedtables.Count() != 0) | ||
| { | ||
| var nestedtable = nestedtables.First(); | ||
| writer.WriteLine(prefix.indents + "<details id=\"nested-table" + nestedtable.index + "\" open>"); |
There was a problem hiding this comment.
good catch. Added closing and additional
pair to allow collapse tables.
| case 70: retValue = (":one: "); break; // one | ||
| case 116: retValue = (":busts_in_silhouette: "); break; // busts_in_silhouette | ||
| case 117: retValue = (":notebook: "); break; // notebook | ||
| case 118: retValue = (":mailbox: "); break; // contact |
| case 64: retValue = (":star: "); break; // custom 1 | ||
| case 70: retValue = (":one: "); break; // one | ||
| case 116: retValue = (":busts_in_silhouette: "); break; // busts_in_silhouette | ||
| case 117: retValue = (":notebook: "); break; // notebook |
|
|
||
| switch (symbol) | ||
| { | ||
| case 3: // to do | ||
| case 8: // client request | ||
| case 12: // schedule/callback | ||
| case 28: // to do prio 1 | ||
| case 71: // to do prio 2 | ||
| case 28: // todo prio 1 |
| } | ||
|
|
||
| prefix.bullets = ""; |
| break; | ||
|
|
||
| case "T": | ||
| { | ||
| var context = DetectQuickStyle(element); | ||
| if (context is not null) | ||
| if (contained) // not in table cell |
There was a problem hiding this comment.
Added DetectQuickStyle again. Let it out as I did not see this actually been used in OneNote so far.
|
|
||
| public PrefixClass(string set_indents = "", string set_tags = "", string set_bullets = "", string tablelistid = "") |
There was a problem hiding this comment.
set_ removed. The default parameters I left equal to allow instantiation with detault parameters.
| } | ||
|
|
||
| if (index >= 0) | ||
| if (element.GetAttributeValue("quickStyleIndex", out int index)) |
There was a problem hiding this comment.
The intention here was to have the check only in the current level for both OE and T as in the intially code. The recursive nature did not work here and it should not be needed, as it is handled at each stage already.
|
@weissm This PR is now far beyond the original intent described by the title. I cannot accept this as a single PR. |
Understood. This PR extension was meant to address: Will create separate PRs to ease handling. |
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
b3c24d7 to
a4e4ada
Compare
|
Reworked the complete change set in cluster changes into indivdiual commits. |
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
a4e4ada to
8bbae19
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
3 times, most recently
from
1160af0 to
bda1f0e
Compare
OneMore/Models/Page.cs
Outdated
| @@ -122,6 +121,33 @@ public void OptimizeForSave(bool keep) | |||
|
|
|||
| public bool IsValid => Root != null; | |||
|
|
|||
| public static List<(string name, string id, string topic, int type)> taglist = new List<(string name, string id, string topic, int type)> | |||
There was a problem hiding this comment.
This list does not belong in the Page model
There was a problem hiding this comment.
I was looking for a common place to handle the taglist in both MarkDownWriter and OneMoreDigExtension. The class page looked like a suitable place. Let me know, which place would fit better.
There was a problem hiding this comment.
Add it as a separate static class under the Markdown folder to keep it within its domain, maybe something like MarkdownEmojis.cs
There was a problem hiding this comment.
Created a dedicated class and file as proposed.
OneMore/Styles/StandardStyles.cs
Outdated
| @@ -39,24 +39,32 @@ public static QuickStyleDef GetDefaults(this StandardStyles key) | |||
| { | |||
| case StandardStyles.Heading1: | |||
| style.FontSize = "16.0"; | |||
| style.SpaceAfter = "0.5"; | |||
There was a problem hiding this comment.
This extension is used from other commands beyond just those related to markdown. Does this adversely affect those? These styles are supposed to emulate the built-in OneMore styles as they appear on the page, not how markdown treats similar style types.
There was a problem hiding this comment.
seperated the spaceafter/spacebefore handling into RewriteHeadings
| .RewriteTodo(touched) | ||
| .SpaceOutParagraphs(touched, 12); | ||
| .RewriteTodo(touched); | ||
| // disabled, as it will space out also short lines. Added space in heading defintion instead |
There was a problem hiding this comment.
See my note below about changing the heading definitions. They need to match the OneNote built-in definitions. This needs to be solved a diffferent way
There was a problem hiding this comment.
Moved the spacebefore/after handling into RewriteHeadings
OneMore/Models/Page.cs
Outdated
| @@ -122,6 +121,33 @@ public void OptimizeForSave(bool keep) | |||
|
|
|||
| public bool IsValid => Root != null; | |||
|
|
|||
| public static List<(string name, string id, string topic, int type)> taglist = new List<(string name, string id, string topic, int type)> | |||
There was a problem hiding this comment.
Add it as a separate static class under the Markdown folder to keep it within its domain, maybe something like MarkdownEmojis.cs
weissm
force-pushed
the
improveMarkdownPullRequest
branch
2 times, most recently
from
5e99072 to
b24e7f6
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
3 times, most recently
from
82429f4 to
ac90721
Compare
added todo handling for tables
added fixes to markdown generation
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
ac90721 to
24f15db
Compare
There was a problem hiding this comment.
Thank you for your work here. I really do appreciate it. I'm going to adopt your changes, except under a different branch so I can incorporate some of my own changes. I've left some comments here to explain those changes. I'll merge to main soon. Best Regards.
| @@ -51,6 +52,28 @@ public void RewriteHeadings() | |||
| { | |||
| RewriteHeadings(outline.Descendants(ns + "OE")); | |||
| } | |||
|
|
|||
| // added header spacing specific to markdown | |||
| var quickstyles = page.Root.Elements(ns + "QuickStyleDef"); | |||
There was a problem hiding this comment.
This is a bad assumption. Should only affect Outline where we are converting markdown. Should abide by existing quick style definitions for page. If user wants spacing, they can create their own custom OneMore Style set and apply that after the conversion.
| /// on the page | ||
| /// </summary> | ||
| public void RewriteTodo() | ||
| { |
There was a problem hiding this comment.
Here also, should only affect Outlines that were affected, not all Outlines on page. User can apply custom style after if they want, otherwise abide by existing styles on page.
| @@ -158,21 +195,43 @@ public MarkdownConverter RewriteTodo(IEnumerable<XElement> paragraphs) | |||
| { | |||
| var cdata = run.GetCData(); | |||
| var wrapper = cdata.GetWrapper(); | |||
| if (wrapper.FirstNode is XText) | |||
| { | |||
| cdata.Value = wrapper.GetInnerXml(); | |||
There was a problem hiding this comment.
This does absolutely nothing? Convert cdata to wrapper and then set cdata to wrapper XML? That's the same thing.
| @@ -204,7 +263,7 @@ public MarkdownConverter SpaceOutParagraphs(float spaceAfter) | |||
| public MarkdownConverter SpaceOutParagraphs( | |||
| IEnumerable<XElement> paragraphs, float spaceAfter) | |||
| { | |||
| var after = $"{spaceAfter:0.0}"; | |||
| var after = spaceAfter.ToString("####0.00", CultureInfo.InvariantCulture); | |||
There was a problem hiding this comment.
We have an extension method, spaceAfter.ToInvariantString()
| /// <summary> | ||
| /// Extended version from RemindCommand to handle also non Todo Tags | ||
| /// </summary> | ||
| public string SetTag(XElement paragraph, string tagSymbol, string tagName, int tagType = 0, bool tagStatus = false) |
There was a problem hiding this comment.
I'm moving this to PageEditor class. Trying to keep the Page class focused on page-level properties and definitions and let PageEditor modify the content of the page.
| @@ -600,6 +603,7 @@ private async Task ImportMarkdownFile(string filepath, CancellationToken token) | |||
|
|
|||
| converter = new MarkdownConverter(page); | |||
| converter.RewriteHeadings(); | |||
| converter.RewriteTodo(); | |||
There was a problem hiding this comment.
See my comment on this method. Should not affect entire page. I'm removing this line.
|
Should be possible to move it into one loop. This structure is applied more for historical reasons.
From: Steven ***@***.***>
Sent: Saturday, January 11, 2025 8:09 PM
To: stevencohn/OneMore ***@***.***>
Cc: Matthias Weiss ***@***.***>; Mention ***@***.***>
Subject: Re: [stevencohn/OneMore] Markdown improvements regarding tables, nested statements and special character support in links (PR #1647)
@stevencohn commented on this pull request.
_____
In OneMore/Commands/File/Markdown/MarkdownConverter.cs <#1647 (comment)> :
-
- // ensure TagDef exists
- var index = page.AddTagDef("3", "To Do", 4);
-
- // inject tag prior to run
- run.AddBeforeSelf(new Tag(index, match.Groups["x"].Value == "x"));
+ var org = text.Value;
+ var completed = match.Groups["x"].Value == "x";
+ text.Value = text.Value.Replace((completed ? "[x]" : "[ ]"), "");
+ cdata.Value = cdata.Value.Replace(org, text.Value);
+ page.SetTag(paragraph, tagSymbol: "3", tagStatus:completed,tagName:"todo");
+ }
+ else
+ {
+ // look for all other tags
+ foreach (var t in MarkdownEmojis.taglist)
You're updating ToDo tags, or all other tags, but not both. Should this all be collapsed down into a single loop that just touches all tags? Then you can handle todo tags inside that loop while still addressing all others too.
—
Reply to this email directly, view it on GitHub <#1647 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAGSHA5RR6O2Y42PQVQMZD2KFT43AVCNFSM6AAAAABQ7AG2W2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNBVGEZTGMZRHA> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/ABAGSHHZB24RZFTXY7M3YUD2KFT43A5CNFSM6AAAAABQ7AG2W2WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUXWOTQM.gif> Message ID: ***@***.*** ***@***.***> >
|
IS YOUR COMMIT SIGNED?
yes
Enhancement or Defect Addressed by This PR
Enhancement
Description of Proposed Changes
When generating markdown nested tables the handled was not done properly. Also the handling of numbered list in tables was not properly processed.
The following changes are applied:
<>. To protect these characters, theReplacestatement is moved before.The push requested was tested on the OneNote TextToTable. For clarification another example onenote page NestedTableContent is added.