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
2 times, most recently
from
367d126 to
82429f4
Compare
added todo handling for tables
added fixes to markdown generation
weissm
force-pushed
the
improveMarkdownPullRequest
branch
from
82429f4 to
ac90721
Compare
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.