Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6220705
src: Allow windows to be transparent, behind a pref (off by default)
DeeDeeG Apr 18, 2024
83f537c
Merge branch 'master' into allow-window-transparency
DeeDeeG Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/config-schema.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,12 @@ const configSchema = {
description: 'When changing the theme within Pulsar also change the theme of the window on the operating system.',
type: 'boolean',
default: false
},
allowWindowTransparency: {
type: 'boolean',
default: false,
title: 'Allow Window Transparency',
description: `Allows editor windows to be see-through. When this setting is enabled, UI themes and user stylesheets can use background colors with an alpha channel to make editor windows translucent. Takes effect after a restart of Pulsar.`
}
}
},
Expand Down
3 changes: 3 additions & 0 deletions src/main-process/atom-application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@ module.exports = class AtomApplication extends EventEmitter {
this.config.onDidChange('core.colorProfile', () =>
this.promptForRestart()
);
this.config.onDidChange('core.allowWindowTransparency', () =>
this.promptForRestart()
);
});
await this.configFilePromise;
}
Expand Down
4 changes: 4 additions & 0 deletions src/main-process/atom-window.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ module.exports = class AtomWindow extends EventEmitter {
options.titleBarStyle = 'hiddenInset';
if (this.shouldHideTitleBar()) options.frame = false;

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

@savetheclocktower savetheclocktower Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@DeeDeeG DeeDeeG Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@DeeDeeG DeeDeeG Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

options.transparent = true;
}

const BrowserWindowConstructor =
settings.browserWindowConstructor || BrowserWindow;
this.browserWindow = new BrowserWindowConstructor(options);
Expand Down
Loading