[core & settings-view] Avoid network requests for bundled packages

[confused-Techie]

In this PR I've made three changes:

• Added atom.branding.urlCoreRepo to equal the value of our package.jsons repository.url
• Don't hit the GitHub API for getting our org picture for settings-view, instead just load the pulsar.png that comes with every installation
• Don't hit the backend API for getting download counts and star counts, instead just skip loading them totally

These changes are all focused on reducing the amount of times we hit the backend unnecessarily. As nobody is really installing our bundled packages, it doesn't make much sense to show download counts or stargazer counts for them. Additionally, loading a locally stored image reduces the amount of network requests needed for every user.

[DeeDeeG]

As a general comment on the idea of using resources/pulsar.png, it does change the aesthetics a bit. Since our GitHub org icon and the resources/pulsar.png app icon are a bit different.

Screenshots:

Before:

After:

I kinda like the org icon? Maybe we can provide that for use here, such as at file path resources/pulsar-edit-org.png?

Not sure where this came from, honestly, but here's a link to a full-size copy of the org icon: https://avatars.githubusercontent.com/u/108445160?s=400&u=cbf7df562590d175c86cc4544194ad0e88933051&v=4

But aesthetics are subjective.

The purple circle background one looks kinda crunchy tho, due to being shrunk down so much and having finer (proportionally smaller in the image) details in it, so if using the purple circle background one, maybe we provide one optimized for this smaller size on-screen?

(This is a moot point if the path resolution isn't fixed in a cross-platform manner, see specific code comment below for addressing that issue, might be a macOS-specific issue, IDK, but I provided a fix with code suggestions.)

Meant to dismiss previous "Changes requested" status, but there is still a fix needed on macOS, so de-escalating my review status to "Comment" until that can be resolved.

[confused-Techie]

@DeeDeeG So I personally like our icon that's in use here more, a while ago I intended on actually using the icon from within the app (Which I think is essentially our semi-official icon) Or at least it's used in the majority of places.

So while I do prefer this one, I do agree it gets a bit "crunchy". So if we would rather use a properly scaled icon, we can ensure that this icon will always be 24px x 24px. So we could potentially offer an optimized version if we would like.

Although I wouldn't be against packaging the GitHub org icon within our binaries themselves, if we think we would prefer it

[DeeDeeG]

I don't want to get too stuck on the aesthetics thing if this saves us a large amount of backend resource usage. I think it's fair to come back to this icon eventually.

Maybe as a follow-up thing, we can run a quick poll -- since it's an aesthetics thing and not too hard to grasp/little context needed, might be the kind of thing that'd be easy to poll about.

[DeeDeeG]

I noticed the download cloud icon doesn't make a ton of sense with no download count, so if we can find a way to remove that when not showing a count, that'd be neat.

Might start to be another thing that'd be good as a follow-up task, I kind of leave the scope of this PR up to your discretion, personally? Others might press harder on the design faux-pas of "indicating something without indicating something" -- a potential source of user confusion, and might be perceived as a lack of polish.

So it's down to, "how serious is design stuff, anyway??" (And I'd plop a code suggestion down if I had found where to change this yet. Probably in a different file?)

EDIT: this.refs.downloadIcon.classList.remove('icon-cloud-download') seems to be the way it is already being done in various places in the code, even from before this PR, if I could find the right place in the code to do it.

[confused-Techie]

@DeeDeeG As you know, I'm not very design oriented. So I'd be very happy with leaving the results of iconography up to a poll, and for the meantime we go with the change as it currently is.

As for the cloud showing with the download count, I do agree it's confusing, but this isn't behavior I've created. There was already a logic flow to not show a download count, which is used for packages that are locally installed, or if Pulsar is unable to reach the package backend. We just add the other if to say if this is a package that's from core. So this behavior of the empty cloud icon isn't new. We are just using it as more than a fallback.

In which case I do agree it's probably best to not show it at all, but being honest, I think we'd either have to optionally display it based on our assigned download count, or otherwise move the logic of creating that part of the display into the same code that adds the download count to the text. No matter what meaning it's a bit more of a change.

I'd be happy adding this as a followup, but with time needed on design issues I'm not sure I could get it done prior to a new release going out. But if we do think it gives to much a sense of imperfection or bad behavior, I'd be willing to wait for this PR to be merged for that behavior.

[DeeDeeG]

Oh, okay. Well, that explains something. Sorry to report this, for me I think it's only "not hitting the API" for locally installed ppm linked or "ppm installed from a git URL" packages.

The rest of my bundled core packages get the GitHub org icon, And even when I rename my cache dir, it is repopulating the cache entries and fetching the GitHub org icon again.

I only confirm the PR's intent working for my two ppm linked packages I was hacking on.

[confused-Techie]

Oh, okay. Well, that explains something. Sorry to report this, for me I think it's only "not hitting the API" for locally installed ppm linked or "ppm installed from a git URL" packages.

The rest of my bundled core packages get the GitHub org icon, And even when I rename my cache dir, it is repopulating the cache entries and fetching the GitHub org icon again.

I only confirm the PR's intent working for my two ppm linked packages I was hacking on.

Well that is rather disappointing. If I had to guess this.pack.repository doesn't contain the raw string of the repository for our core packages like I'd hoped.

I'll have to take another look at this, and hope we can get it in in time. Otherwise if we can't, at the very least we will be able to properly touch on some of the other aspects we care about here. Thanks for letting me know

[DeeDeeG]

I did some quick and dirty console.log()ing, and got this:
this.pack.repository: [object Object] did not match atom.branding.urlCoreRepo: https://github.com/pulsar-edit/pulsar

It's returning the object form, for the usual bundled packages, I reckon.

So there should be some convenience function somewhere to grab the repository.url, or else we can write one without too much trouble if typeof this.pack.repository === 'object' or something like that.

And we do want to do it dynamically like that, handle objects and strings both correctly, since the ppm linked packages are returning the string form of pack.repository, for whatever reason.

[confused-Techie]

this.pack.repository: [object Object] did not match atom.branding.urlCoreRepo: https://github.com/pulsar-edit/pulsar

It's returning the object form, I reckon.

So there should be some convenience function somewhere to grab the repository.url, or else we can write one without too much trouble if typeof this.pack.repository === 'object' or something like that.

Great find! There is already a helper function ownerFromRepository() but that only returns the owner, so I can make another helper that does what we describe here, to ensure we are always getting the repository. Super awesome, I'll make that change in just a moment

[confused-Techie]

And done, lets see if that does it

[DeeDeeG]

Argh, it's adding .git at the end???

this.pack.repository, JSON.stringify()ed, is: {"type":"git","url":"https://github.com/pulsar-edit/pulsar.git"}

Anyhow, that's the format it's giving. All the bundled packages show similar in my console.log()s. (Sorry I assumed it would be the same as in the packages' package.jsons, as in "without the .git at the end.")

EDIT: And just to be clear, it's not adding .git at the end for ppm linked packages. Just the bundled packages or ones installed from the package backend.

So, for the string type repositorys, we shouldn't normalize to add a .git. Only for the object style ones.

EDIT 2: Although technically someone could hard-code that with the .git actually in the package.json, not that we should be in the habit of doing that for core packages.

Maybe we should do repoUrlFromRepository(this.pack.repository).includes(atom.branding.urlCoreRepo)??

[confused-Techie]

Argh, it's adding .git at the end???

this.pack.repository, JSON.stringify()ed, is: {"type":"git","url":"https://github.com/pulsar-edit/pulsar.git"}

Anyhow, that's the format it's giving. All the bundled packages show similar in my console.log()s. (Sorry I assumed it would be the same as in the packages' package.jsons, as in "without the .git at the end.")

So did I. Later on I'll have to look into exactly how this is generated, since that just seems so odd. But we can work with that, just a sec lol

[Spiker985]

Not a discussion for this change, but on the note of "crunchy" icons, (being that it is very late in the evening/very early in the morning and not from my computer) do we not ship an svg?

I know for a fact we ship a png, or collection of icons. But arguably, we could (and should, if we already don't) ship the svg due to linux at the very least.

That would be a much better resource to use a static call to, as svgs are natively scalable due to their vector nature

[confused-Techie]

@Spiker985 So looking at our electron-builder config we do in fact ship an SVG icon on Linux only.

So in the long term you are totally right, that instead of serving variations of the same icon, at least in usage like this we should just provide an SVG.

[DeeDeeG]

It's working!

settings-view.pane.showing.installed.packages.--.scrolling.and.seeing.different.icons.for.each.package.dependending.on.the.repository.url.mostly.the.white.pulsar.logo.on.a.purple.background.three.with.the.purple.logo.no.background.one.atom.org.logo.mov

I do see a mix of logos, mostly the resources/pulsar.png (white pulsar logo on a purple circle background, kinda cronchy), three packages with the GitHub org logo (purple pulsar logo with no background), and one Atom org logo as before.

And packages installed from git will of course use the org/account avatar from their respective repositories, assuming the repo isn't our core repo URL, which generally it wouldn't be.

Comfortable merging this on the technical basis, style questions aside for a later time.

[Spiker985]

Do we want to get this merged for 109, and open a follow-up issue for additional adjustments?

[DeeDeeG]

Would love to see this merged for 109, I fully intend for it to be included, personally.

And then for a separate GitHub issue about the design + style stuff, I might lose track of the discussion in an issue, personally, and I was gonna open another thread on #pulsar-core on Discord for it?

UPDATE: thread posted on Discord for the follow-up/design discussion: https://discord.com/channels/992103415163396136/1152330859702136953

[Spiker985]

Oh, seeing how this is 42 commits behind - should we get it up to date just to make sure something didn't break it in those 42 commits - then if it succeeds: merge for 109?

[confused-Techie]

@Spiker985 I think we are still good to merge on this one, as I'm pretty sure the only thing merged between when I created the branch and now was the August Tree Sitter fixes. So nothing that should effect us here. Unless you wanted to be sure we could, otherwise, would love to get this in for 109, and we can then later focus on design aspects