search

JetBrains / markdown

Markdown parser written in kotlin
Apache License 2.0
682 stars 75 forks source link

Emit absolute URLs instead of relative when rendering links and images #10

Closed sdeken closed 8 years ago

sdeken commented 8 years ago

Produce absolute paths to relative URIs while rendering links and images.

I'm not 100% happy with this, it feels like I had to add the base URI in too many places, but here we are.

valich commented 8 years ago

Hi @sdeken,

Sorry for my being slow, I have been trying to understand why I don't like this URI parameter everywhere and finally I understood that it's in the right place actually. I have added a couple more tests and made a little refactoring to the HtmlGenerator constructor because it was already bad. It all've been pushed to a base-uri branch.

The thing is that that smart URI#resolve thing (org/intellij/markdown/html/GeneratingProviders.kt:191) may easily throw an exception so one needs to wrap it somehow. I think, one may catch IAE with just returning info.destination there. What do you think?

sdeken commented 8 years ago

How's this look?

I've refactored the URL resolution into a single function, and wrapped it in a try..catch in case it does throw an exception.

valich commented 8 years ago

Yep, that works, however, I've added a test to reproduce a potential to-be-failure. BTW I have not noticed code duplication of the url resolution previously :)

Thanks Stephen! I'll update the plugin with this feature soon.

holgerbrandl commented 8 years ago

I've tried the latest master including this PR with the latest intellij-community master. However, I still struggle to get it working since the baseURI is always null in org/intellij/markdown/html/HtmlGenerator.kt:19. Doesn't org.intellij.markdown.html.HtmlGenerator need to expose the baseURI as a constructor parameter?

valich commented 8 years ago

There was refactoring of constructor: the idea was to build htmlGeneratingProviders outside of the constructor and pass them to it. The reason was that Flavour, LinkMap (and baseURI now) are all not really needed for other than constructing providers. See this commit for usage change example.

sdeken commented 8 years ago

I just updated. Looks awesome! I've been doing this professionally for like 15 years now, but I still feel like a little kid every time I see code that I wrote make it out into the real world.

I initially had a problem with the new build of the Markdown plugin throwing an exception when saving the configuration. Upgrading from IDEA Community 15.0.5 build #IC-143.2332 to 2016.1.1 build #IC-145.597 resolved that problem, though.

Thanks for your help, valich!

valich commented 8 years ago

@sdeken :)

BTW if you encounter any bugs in the plugin, don't hesitate to report them in the tracker. If there is anything serious, it should be handled swiftly.

sdeken commented 8 years ago

@valich, I hate to be a bother, but could you walk me through the process of building the intellij-plugins? Specifically markdown-javafx-preview. There's another issue I've just run into in which the JavaFX previewer caches the images and refuses to reload them when changes have been made, but I can't track down the problem without being able to build the plugins.