src: Allow windows to be transparent, behind a pref (off by default)

[DeeDeeG]

Issue or RFC Endorsed by Pulsar's Maintainers

resolves #948.

Was discussed in the issue above, and further discussed on Discord:

• https://discord.com/channels/992103415163396136/992109539346370661/1223052096803438693
• https://discord.com/channels/992103415163396136/1230350567176405053

Description of the Change

Add a core.allowWindowTransparency pref (off by default).

Sets options.transparent = true for the browserWindow options when this pref is enabled.

Requires a restart to activate.

Alternate Designs

Could put this on by default, or even always on without a pref to control it. I wanted it off by default just to be on the cautious side about it, in case it breaks the app / its rendering for somebody out there, or perhaps there is some perf hit (not investigated).

Could not implement the requested transparency mode at all, but this seems low-stakes enough behind a pref that's off by default, kind of a "why not?" thing at this point.

Could make the pref not prompt for restart when editing it in the settings-view UI, but... why not prompt for restart? (I think we do poll the preferences very frequently, and there is a listener for changes to certain prefs at which point it will prompt for a restart, but that is an existing pattern. If it's worth revisiting that for performance or wanting to reduce I/O, we can revisit that separately?)

Possible Drawbacks

None that I know of.

Verification Process

Tested, worked for me on macOS. (Kind of weird-looking with the CSS the original guide provided, but if this is what people are requesting, this is it. You can indeed see the apps/desktop behind it. Maybe enabling this situationally like when the editor is not focused would make sense. Theme authors and user stylesheet authors have lots of freedom to do whatever they want with it.)

(Note that the lack of a frame (window title bar) in these screenshots is technically unrelated. At the time I took these screenshots, I had set some options to disable the frame per the atom-transparency guide, but that option has been made its own pref since the original atom-transparency guide was published, so lack of window titlebar/"frame" is not part of this PR. Folks following that old guide can now use the separate, existing core.titleBar pref to control that.)

Release Notes

Add a preference core.allowWindowTransparency so that themes and user stylesheets can make editor windows' backgrounds transparent.

[savetheclocktower]

Suggested change

	if(this.atomApplication.config.get('core.allowWindowTransparency')){
	if (this.atomApplication.config.get('core.allowWindowTransparency')) {

[savetheclocktower]

Actually, what if we turned this into a method like the ones above?

if (this.shouldAllowWindowTransparency()) options.transparent = true;

Then we can define a method like

shouldAllowWindowTransparency() {
  return (
    !this.isSpec && 
    this.atomApplication.config.get('core.allowWindowTransparency');
  );
}

I actually did some code spelunking just now to see if anyone ever discussed why application of these window settings is suppressed when Atom ran in spec mode, but I couldn't find anything relevant. Still, I'm assuming that there's a good reason behind it.

[DeeDeeG]

I actually did some code spelunking just now to see if anyone ever discussed why application of these window settings is suppressed when Atom ran in spec mode, but I couldn't find anything relevant.

This piqued my curiosity.

How to find the info

Tracing back through the git-blame, we hit a decaf commit where the filename transitioned from src/main-process/atom-window.coffee to src/main-process/atom-window.js. Looking at the blame view on the .coffee file, as of the commit just before the decaf commit (last commit the .coffee file existed), and hopping over to atom/atom repo on github.com to make sure the PRs the commits are from are annotated properly on the page for the individual commit, we get this commit as the earliest to introduce the isSpec check: atom/atom@08a3582, which is from this PR: atom/atom#11790

Thoughts:

I'm not quite sure if it's explained well there either. But I thought I'd post these notes for now while I'm looking for an explanation.

---

EDIT: This commit message feels slightly relevant at least?

check for on method

during window-event-handler-spec, applicationDelegate.getCurrentWindow
seems to return a truncated version of a browserWindow object, which does not include the on method

I don't know if that was accurate at the time, whether it's still true or not, or if it's related to wanting to turn these toggles off during specs.

[DeeDeeG]

I'm personally (a lot) more inclined to make these prefs operational during specs if possible, since I'd want to test as like-for-like code paths in CI as in real production use as possible, if there is no incompatibility or blocker for doing so.

[savetheclocktower]

The test failure here was spurious (known flaky test) but this'll need a rebase against latest master for CI to start working again.

[DeeDeeG]

Turns out the "package tests" workflow is Linux-only, so we don't need the "pin to macOS 12" fix here. The only jobs needing re-runs were on Linux exclusively.

CI is passing/green now!

[DeeDeeG]

Should I re-do the implementation to match the others, per #982 (comment)?

Or is this good to go now that CI is green/passing?

(If it makes sense to "re-enable all the things [during specs]", then I suppose that could be punted to another PR. In favor of that would be uniformity. Against that would be the argument that such a follow-up PR is low-stakes enough to probably not justify the effort, so it may not happen.

[savetheclocktower]

Should I re-do the implementation to match the others, per #982 (comment)?

Or is this good to go now that CI is green/passing?

It's up to you. Was a suggestion, but certainly not a blocker.

[DeeDeeG]

Thanks for the review!