For evaluation: Implementation of MarkdownDeep.net over Journaley. #129

crxtrdude · 2015-11-23T15:33:49Z

Replaced MarkdownSharp with MarkdownDeep.net. Has better compatibility with some Markdown features that Day One supports including Markdown Extra features (tables and footnotes for one), although there are some certain things it doesn't have. That's where fixes comes in, the most important is for single line breaks.

For more information, refer to the commits for details.

- Replaced MarkdownSharp with MarkdownDeep.net. Has better compatibility with some Markdown features that Day One supports - Added a fix for single line breaks that isn't implemented in MarkdownDeep. Note that this needs some optimization.

crxtrdude · 2015-11-23T15:34:49Z

Journaley/Forms/MainForm.cs

            string formattedString = string.Format(
-                    "<style type=\"text/css\">\n<!-- Font CSS -->\n{0}\n<!-- Size CSS -->\n{1}\n<!-- Custom CSS -->\n{2}\n</style><html><body><div>{3}</div></body></html>",
+                    "<style type=\"text/css\">\n<!-- Font CSS -->\n{0}\n<!-- Size CSS -->\n{1}\n<!-- Custom CSS -->\n{2}\n</style>\n<html>\n<body>\n<div>\n{3}\n</div>\n</body>\n</html>",


Added proper line breaks for the tags.

- Transferred the fix into its own helper class, PostMarkdownParser. - Added fixes for strikethroughs and backtick code blocks (converting the tagged code into <pre>). - Added a DOCTYPE on the formatted HTML string to make the HTML parsed HTML 4 strict.

yyoon · 2015-11-24T07:07:14Z

Journaley.Core/Journaley.Core.csproj

@@ -97,7 +97,9 @@
      <ErrorText>This project references NuGet package(s) that are missing on this computer. Enable NuGet Package Restore to download them.  For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
    </PropertyGroup>
    <Error Condition="!Exists('..\packages\StyleCop.MSBuild.4.7.48.2\build\StyleCop.MSBuild.Targets')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\StyleCop.MSBuild.4.7.48.2\build\StyleCop.MSBuild.Targets'))" />
+    <Error Condition="!Exists('$(SolutionDir)\.nuget\NuGet.targets')" Text="$([System.String]::Format('$(ErrorText)', '$(SolutionDir)\.nuget\NuGet.targets'))" />


Was there a particular reason to change this file?

I really don't know. I think this was when I enabled Nuget Package Recovery in VS.

No, you shouldn't enable that in the solution. In fact, it turns out these changes (in Journaley.Core, Journaley, VersionExtractor) broke the build on my side. Please remove these lines from all the affected projects.

You should just go to Tools -> Options... -> NuGet Package Manager -> General and check both the Allow NuGet to download missing packages and Automatically check for missing pakcages during build in Visual Studio options. The solution should build fine after enabling these options. If not, let's troubleshoot that together later on.

I'll reset my repo and I'll do this again. I'll message you if it worked with just removing the code first.

@yyoon

- Improved PostMarkdown function as suggested by @yyoon. - Reverted screwed up csproj files due to Nuget Package Recovery. - Modified docType string to use HTML5 DOCTYPE standard. - Minor fixes.

yyoon · 2015-11-25T07:16:06Z

Journaley/Utilities/PostMarkdownParser.cs

+            // 2 - stumbles upon a <pre> tag.
+            var parseState = 0;
+
+            string newLineString;


Declare this variable when its used in line 86. This makes it easier to understand the code, because the scope of this variable is actually within the else if (parseState == 0) block. It is fine for the other variables like builder, paragraphBuilder, and parseState, because their state persists over the loops.

Let's rename this to paragraph, because it is a better name, it comes from the paragraphBuilder, and the name newLineString can be misleading because it usually refers to \r\n or some other platform dependent newline string.

yyoon · 2015-11-25T07:35:58Z

Another issue I noticed: when playing with your test file, I found that the entry area shows nested scrollbars like this:

This happens because now we use the <pre> tag for the fenced code, and it doesn't wrap the line by default. We can fix this behavior by adding the CSS.

pre {
    word-wrap: break-word;
}

Please add this to all three css files (small, medium, large).

yyoon · 2015-11-25T15:51:55Z

Okay. That makes sense to me.

yyoon · 2015-11-25T15:53:48Z

Journaley/Utilities/PostMarkdownParser.cs

+
+            // 0 - stumbles upon the beginning of a <p> tag/usual parsing.
+            // 1 - stumbles upon the end of </p> tag.
+            // 2 - skips check and just dumps the line to builder.


I think you meant to say -1 here, not 2.

crxtrdude · 2015-11-25T16:14:58Z

The reason for those two lines is because of something like this:
Supposed you got something like this:

~~Line 1
Line 2
Line 3~~

Obviously it's expected like this:

<p><del>Line 1
Line 2
Line 3<del><p>

Which is what GFM and Day One renders:
Line 1
Line 2
Line 3

For this kind of paragraph, that's why I added those 2 lines on this case.

crxtrdude · 2015-11-25T16:16:43Z

Why do I always press the first button ...

But anyway, I did agree with passing Strikethrough in paragraph, which did work on my side.

yyoon · 2015-11-25T16:34:28Z

I understand why you did that, but I still think it was incorrect for a few reasons.

First, it wouldn't necessarily match the opening / closing strikethrough mark pairs.
Take the following example.

~~Line1
Line2
Line3

GitHub renders it as:

~~Line1
Line2
Line3

, because there is no closing ~~ mark at the end.

However, the current code seems like it would just add the opening <del> and then never close it.
Processing the opening / closing mark separately will cause all sorts of this problems.

Second, it seems to me that it would only handle the ~~ marks that appear right after  and right before  am I right? This may not be able to handle the following case:

Normal text ~~strikethrough
should be applied here~~ and not here.

GitHub renders it as:

Normal text strikethrough
should be applied here and not here.

, but I think our previous code would probably not handle this correctly.

These are the reasons why I suggested to process at the paragraph level, not at the line level.

crxtrdude · 2015-11-25T16:39:40Z

The code I'm doing (not commited yet) is now processing it on the paragraph level now. I switched every line process over to paragraph level.

Your second example BTW, has not been rendered on Github, but it does render it on Day One and since we need to be as close to day one as possible, I might need to have a intelligent way of doing this.

crxtrdude · 2015-11-25T16:40:17Z

Why does the button for closing a PR is the one I always push? It's getting my nerves a bit.

yyoon · 2015-11-25T16:47:50Z

I guess space between ~~ and the text matters:

Normal text ~~strikethrough
should be applied here~~ and not here.

Normal text strikethrough
should be applied here and not here.

Normal text ~~ strikethrough
should be applied here~~ and not here.

Normal text ~~ strikethrough
should be applied here~~ and not here.

crxtrdude · 2015-11-25T16:54:13Z

~~I managed to actually successfully implement it. I will update our Test Syntax to reflect some of these changes. I'll commit it now.~~

crxtrdude · 2015-11-25T17:36:46Z

On second thought, there are problems that are going on. I'll fix this tomorrow morning.

- Removed strikethrough support for now until we can find a way to do it properly (preferably not using Regex). - Single line breaks are now properly added. - Uses StringReader instead of a foreach loop in reading the string. - Backticked code blocks is now properly parsed into <pre> tags.

crxtrdude · 2015-11-26T11:39:49Z

Here's the (hopefully last) implementation of PostMarkdown. I removed strikethrough support for now, because there are a lot of things to consider, but what I wanted to make sure was the other parts, especially single line breaks, are implemented.

If we commit this now, with little to no changes, we'll just add the strikethrough problem as an issue and then work on it incrementally.

- Since we are doing this upon render time, just replace the backticks into tildes instead to make it properly render HTML tags that might be used (HTML is still considered parsed despite converting the tags to <pre>).

crxtrdude · 2015-11-26T12:03:46Z

Sorry, I need to do a followup for this one. Apparently, parsing backtick fenced code blocks post markdown would not escape HTML tags.

This is like similar to what I said (I think I said it) where   could be used inside a <code> block and it will render a newline.

That's why in this fix, instead of having to fight MarkdownDeep, I'll just convert the backtick fence into a tilde fence, that way it would be transformed properly as it is intended. The user's file of course is not affected by it since I only change it on UpdateWebBrowser only.

I'll find a way to do this programmatically on PostMarkdown properly, but the priority here is making the single line break work.

yyoon · 2015-11-26T18:32:49Z

Looks good to me. Thanks!

Implementation of MarkdownDeep.net over Journaley.

crxtrdude reviewed Nov 23, 2015
View reviewed changes

crxtrdude mentioned this pull request Nov 23, 2015

Additional Markdown Support #124

Closed

crxtrdude changed the title ~~Initial implementation of MarkdownDeep.net over Journaley.~~ Implementation of MarkdownDeep.net over Journaley. Nov 24, 2015

yyoon reviewed Nov 24, 2015
View reviewed changes

Additional MarkdownDeep fixes ...

5853c58

- Improved PostMarkdown function as suggested by @yyoon. - Reverted screwed up csproj files due to Nuget Package Recovery. - Modified docType string to use HTML5 DOCTYPE standard. - Minor fixes.

yyoon reviewed Nov 25, 2015
View reviewed changes

crxtrdude changed the title ~~Implementation of MarkdownDeep.net over Journaley.~~ For evaluation: Implementation of MarkdownDeep.net over Journaley. Nov 25, 2015

yyoon reviewed Nov 25, 2015
View reviewed changes

crxtrdude closed this Nov 25, 2015

crxtrdude reopened this Nov 25, 2015

crxtrdude closed this Nov 25, 2015

crxtrdude reopened this Nov 25, 2015

Follow-up commit: Backtick fenced code block fix

490f222

- Since we are doing this upon render time, just replace the backticks into tildes instead to make it properly render HTML tags that might be used (HTML is still considered parsed despite converting the tags to <pre>).

yyoon added a commit that referenced this pull request Nov 26, 2015

Merge pull request #129 from crxtrdude/feature/markdown_deep_2

85e765d

Implementation of MarkdownDeep.net over Journaley.

yyoon merged commit 85e765d into yyoon:develop Nov 26, 2015

crxtrdude deleted the feature/markdown_deep_2 branch November 27, 2015 02:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For evaluation: Implementation of MarkdownDeep.net over Journaley. #129

For evaluation: Implementation of MarkdownDeep.net over Journaley. #129

crxtrdude commented Nov 23, 2015

crxtrdude Nov 23, 2015

yyoon Nov 24, 2015

crxtrdude Nov 24, 2015

yyoon Nov 24, 2015

crxtrdude Nov 24, 2015

yyoon Nov 25, 2015

yyoon commented Nov 25, 2015

yyoon commented Nov 25, 2015

yyoon Nov 25, 2015

crxtrdude commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

yyoon commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

yyoon commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

crxtrdude commented Nov 26, 2015

crxtrdude commented Nov 26, 2015

yyoon commented Nov 26, 2015

For evaluation: Implementation of MarkdownDeep.net over Journaley. #129

For evaluation: Implementation of MarkdownDeep.net over Journaley. #129

Conversation

crxtrdude commented Nov 23, 2015

crxtrdude Nov 23, 2015

Choose a reason for hiding this comment

yyoon Nov 24, 2015

Choose a reason for hiding this comment

crxtrdude Nov 24, 2015

Choose a reason for hiding this comment

yyoon Nov 24, 2015

Choose a reason for hiding this comment

crxtrdude Nov 24, 2015

Choose a reason for hiding this comment

yyoon Nov 25, 2015

Choose a reason for hiding this comment

yyoon commented Nov 25, 2015

yyoon commented Nov 25, 2015

yyoon Nov 25, 2015

Choose a reason for hiding this comment

crxtrdude commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

yyoon commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

yyoon commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

crxtrdude commented Nov 25, 2015

crxtrdude commented Nov 26, 2015

crxtrdude commented Nov 26, 2015

yyoon commented Nov 26, 2015