yyoon / Journaley

A simple and elegant open-source journal keeping software for Windows
http://journaleyapp.com/
Other
115 stars 19 forks source link

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

Closed crxtrdude closed 8 years ago

crxtrdude commented 8 years ago

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.

yyoon commented 8 years ago

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

image

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).

crxtrdude commented 8 years ago

Yeah this does not wrap the line by default, but the problem is not that, it's because the output of:

<code>` spans are delimited by backticks. You can include literal backticks like ``this``.

Which should result into this:

<p><code>&lt;code&gt;</code> spans are delimited by backticks. You can include literal backticks like <code>this</code>.</p>

Became this:

<pre><code>&lt;code&gt;</code> spans are delimited by backticks. You can include literal backticks like <code>this</code>.</p>

And the rest of the output became bad as a result as well. I need to be careful with this, I need to change my code to reflect that.

crxtrdude commented 8 years ago

I have fixed it, actually putting it on the parseState 0 so that it checks in a paragraph instead of a single line only.

Although actually, your commented fix on the <br> actually solved the problem of the <p><code> break. (Since it's enclosed in a <p> tag, even using a code block, <br> applies as one line break. <pre> on the other hand treats <br> as double)

Since people will not check the source of the HTML anyway, we could just omit that

crxtrdude commented 8 years ago

Sorry, pressed the wrong button

yyoon commented 8 years ago

Okay. That makes sense to me.

crxtrdude commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 <p> and right before </p> 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 commented 8 years ago

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 commented 8 years ago

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

yyoon commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

crxtrdude commented 8 years ago

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.

crxtrdude commented 8 years ago

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 <br /> 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 commented 8 years ago

Looks good to me. Thanks!