Markdown.Editor / Pagedown cleanup. Option to disable preview events. (PR #477)

Notes

  • cleaned up some variable caching and newlines / if blocks / strict equality comparisons.
  • added option to disable automatic preview events
  • added callback to html-sanitizer-bundle script load to ensure preview is generated on page load
  • fixed a double timeout inside the refresh method (applyTimeout on a debounced method).

Commit message

Markdown.Editor cleanup. cached setScrollPanelTops calculations. Added option to set preview to manual, won’t bind input events. removed double timeout call in refresh (applyTimeout + debounce). added wait callback in pagedown_editor to sanitizer script to refresh preview on load.

GitHub

You’ve signed the CLA, awbergs. Thank you! This pull request is ready for review.

Overall I am happy with this change, but do not want us to diverge from the official Pagedown source, can we get the prep work applied upstream first?

  • the manualPreview change is a good idea, however I don’t understand the changes in L994 and following (also the new constructor option isn’t documented; I can make that change though.).
  • the getScaleFactor is okay I guess, although unlike the commit message says, it’s not caching calculations, it’s just caching lookups. No sane browser would calculate those values more than once.

The rest of the changes aren’t in upstream code, so I won’t comment on them.

I have to wonder though why all of this is crammed into a single commit?

@SamSaffron I can definitely get that done upstream first.

@balpha -The way I thought about it was that manualPreview means “let the caller be in control of when to refresh” so I didn’t think it made sense to, when passing the manualPreview parameter in, call applyTimeout. -My bad about the wording for getScaleFactor. Caching the lookup is what I meant. -The only change that’s not really related to the manualPreview is the lookup caching so I didn’t feel like I was trying to cram a bunch of stuff into a single commit. It can definitely be broken up a tiny bit but it was all in the same file and each piece was in the codepath for preview so I figured why not.

Also after sleeping on it and thinking about it some more I think it would be better to make the manualPreview parameter a function instead of a boolean. The boolean seems sort of like a workaround or a hacky option while a function would be an enhancement. Also there is the added benefit that we would be able to set our manualPreview option to a Discourse.debounced function instead of having the ad-hoc debounce function inside of PreviewManager.

I take that back. A boolean to disable the auto preview refresh makes more sense. Working on the upstream work right now.

@SamSaffron Are you referring to https://code.google.com/r/samsaffron-pagedown? I’m having a hard time finding which official Pagedown source we would be diverging from.

The Markdown.Editor.js file in discourse is quite different from the /r/pagedown repo as well as from the /r/samsaffron-pagedown repo.

I want to do the right thing and submit a patch to the official source but if I copy in the source from either of those repos there are way more differences than I’m comfortable with bringing in.

@awbergs the official pagedown is at: https://code.google.com/p/pagedown/ … we should be running latest from there as @balpha recently merged us in.

I’ll defer to @balpha for what course to take. I can patch from https://code.google.com/p/pagedown but there are a bunch of differences between the file in source and the official Pagedown source, not including the old debouncing function work.

@awbergs I really thought we should have been in-line, any chance you can patch https://code.google.com/p/pagedown so we are totally in sync?

@SamSaffron Yeah I can do that.

The way I thought about it was that manualPreview means “let the caller be in control of when to refresh” so I didn’t think it made sense to, when passing the manualPreview parameter in, call applyTimeout.

But “be in control” to me means mean calling editor.refreshPreview(). That proxies to previewManager.refresh(true);, which is synchronous and doesn’t use a timeout.

@balpha I see what you’re saying now. That check inside of refresh is totally unnecessary. I’ll take it out when I resubmit the PR.

@awbergs any news here?

@SamSaffron I have been pretty busy with some other things for the past few weeks so I haven’t had any time to work on this. I should be able to get back into it this upcoming weekend.

I can’t leave this open forever :slight_smile: should I close it for now. I can’t take PRs that touch externals anyway.

closing for now … can’t leave this open any more …