Markdown improvements regarding tables, nested statements and special character support in links#1647
Markdown improvements regarding tables, nested statements and special character support in links#1647weissm wants to merge 19 commits intostevencohn:mainfrom
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 string bullets = ""; | ||
| public string tablelistid = ""; | ||
|
|
||
| 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.
| { | ||
| 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.
| 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.
weissm
left a comment
weissm
left a comment
There was a problem hiding this comment.
Worked in changes as suggested.
| 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 |
| case 12: // schedule/callback | ||
| case 28: // to do prio 1 | ||
| case 71: // to do prio 2 | ||
| case 28: // todo prio 1 |
| writer.Write("</li>"); | ||
| } | ||
|
|
||
| prefix.bullets = ""; |
| { | ||
| 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 string bullets = ""; | ||
| public string tablelistid = ""; | ||
|
|
||
| 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
|
|
||
| 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
| { | ||
| 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
|
|
||
| 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
4 times, most recently
from
ac90721 to
24f15db
Compare
stevencohn
left a comment
stevencohn
left a comment
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.
| } | ||
|
|
||
| // 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.
| 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.
| 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.
|
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: ***@***.*** ***@***.***> >
|
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
24f15db to
c87cb38
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
c87cb38 to
e7e944d
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
3 times, most recently
from
e929b88 to
5ce9f7e
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
2 times, most recently
from
850c6ab to
dd3666c
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
2 times, most recently
from
931ecd8 to
61235e6
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
61235e6 to
4515de5
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
2 times, most recently
from
f6ede11 to
ea2393a
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
ea2393a to
6eb7f8d
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
6eb7f8d to
154842a
Compare
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
154842a to
e039c3b
Compare
added todo handling for tables Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
…tyles instead Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
added fixes to markdown generation Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
…quickstyle Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
…t setting) Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
Signed-off-by: Matthias Weiss <matthias.h.weiss@arcor.de>
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
e039c3b to
c7ee030
Compare
2 participants
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.