Fix markdown image src transformation in some usages

[savetheclocktower]

The problem

In the terminal package’s README, I've got a screenshot to demonstrate the functionality. I was inclined to do what other builtin package READMEs have done and use GitHub as an image host. But I think a lot of those tricks no longer work! Try as I might, I can't get an old-style cloud.github.com/assets URL when I upload an attachment to a GitHub issue; they're all URLs that strongly imply that they are not permanent and may expire at some point in the future. (Probably to cut down on people using GitHub as an image host.)

The next best thing is to check the screenshot into source control and reference it from the README as a relative URL; this works fine on GitHub itself. But the image was showing up as broken on the terminal package’s settings page (we show the package's README after all the settings, keybindings, and whatnot).

I dug into why this was happening and it was mostly some spaghetti in a section of atom.ui.markdown.render that doesn't have any test coverage. I have taken the cowardly path and failed to add to that test coverage, but I do think this code works better than it did before.

The fix

When you call atom.ui.markdown.render, you can specify a rootDomain (if your Markdown came from a remote URL) or a filePath (if your Markdown came from a file on disk). That function has a section that handles the possible need to transform image src values (relative src values, etc.), but that section was being skipped entirely when rootDomain was omitted. This meant that settings-view’s PackageReadmeView didn't get the benefit of image src transformation at all.

Once that was fixed, there was more to solve, including an accidental negation in a conditional, and a reference to a nonexistent variable.

We also did something a bit strange: if rootDomain was specified at all, we tried a way of resolving the image that would only work if rootDomain referred to a GitHub URL. Instead, I made it so that that tactic was only attempted if we actually saw github. in rootDomain; otherwise I treated it like an arbitrary URL and combined rootDomain and src to produce an absolute URL.

Testing

This PR is against master, but I don't have a great way of testing this until the Electron 30 bump lands — because my test package is terminal, which isn't written to be compatible with Electron 12.

I'll see if I can update one of my other packages so that the README references a screenshot that lives in the repo. Then someone can install it via GitHub and verify that way.

[savetheclocktower]

OK, this should let you verify this PR:

ppm install savetheclocktower/pulsar-hover

Then open Settings -> Packages -> pulsar-hover and click on Settings. Scroll past the settings to see the README and verify that the image doesn't show up as broken.

[savetheclocktower]

Meanwhile, I'll update terminal’s README so that it no longer exhibits this bug. I remembered what I'd done for pulsar-hover: once the screenshot lives in the repo, you do get a URL which you can use to refer to it in the long term. That's the approach that works better for the package registry (so it doesn't have to internalize all of this logic when parsing Markdown in README.md files!).

[savetheclocktower]

Pinging @confused-Techie for review.

[confused-Techie]

Gonna just provide an approval here, as it's been sitting for far too long for in-depth tests, and otherwise looks solid.

I will admit, when I originally wrote this it was mostly just on tracking things in my head without proper tests coverage, although eventually I'd love to add some proper tests here, or even find a way to space this out and let things make a bit more sense.

In any case, I totally agree, this probably works better now than it ever has.

[DeeDeeG]

Tangential to this PR, I got service.addModifierProvider is not a function when trying to activate pulsar-hover package.

PR review incoming.

[DeeDeeG]

I can verify the steps to reproduce in #1375 (comment) are now working with this PR's CI binary.

(And I confirm it was broken on the latest Rolling, so that indicates the fix is indeed needed.)

👍

[DeeDeeG]

I am honestly a bit suspicious of trusting various web URLs to pull image files from and render them, but that's probably not the main/only concern with such an extensible/"hackable" design as Atom/Pulsar, so eh? Anyhow. Just something that crossed my mind.

And this PR doesn't introduce that concept, just tweak its implementation, so... Probably not something relevant to hold this particular PR up over.

[DeeDeeG]

Is this spec failure real? It failed three times and seems possibly genuinely related to the subject matter of the PR.

MarkdownPreviewView
  text selections
    it adds the `has-selection` class to the preview depending on if there is a text selection
      TypeError: Failed to execute 'selectAllChildren' on 'Selection': parameter 1 is not of type 'Node'.
        at jasmine.Spec.<anonymous> (/home/runner/work/pulsar/pulsar/node_modules/markdown-preview/spec/markdown-preview-view-spec.js:360:17)
        at jasmine.Spec.args.<computed> (/opt/Pulsar/resources/app.asar/spec/runners/jasmine1-test-runner.js:105:31)
  when GitHub styles are enabled
    it uses the GitHub styles
      TypeError: Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'.
        at jasmine.Spec.<anonymous> (/home/runner/work/pulsar/pulsar/node_modules/markdown-preview/spec/markdown-preview-view-spec.js:628:14)
        at jasmine.Spec.args.<computed> (/opt/Pulsar/resources/app.asar/spec/runners/jasmine1-test-runner.js:105:31)


ALL TESTS THAT FAILED:
MarkdownPreviewView text selections adds the `has-selection` class to the preview depending on if there is a text selection
MarkdownPreviewView when GitHub styles are enabled uses the GitHub styles

Edit: On closer look, hopefully not related. Edit 2: ? I dunno.

[savetheclocktower]

Yeah, probably legit. No hurry on this PR; I'll take a look.