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.

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

Multi-monitor support #11

Merged
merged 18 commits into from
Jan 17, 2022
Merged

Multi-monitor support #11

merged 18 commits into from
Jan 17, 2022

Conversation

s0
Copy link
Contributor

@s0 s0 commented Jan 14, 2022

closes #10

There are a couple of commits I also added to ease development a little bit (638c852, 0d16e9d), I can more them to another PR or just remove if you'd prefer.

This is an initial implementation for multi-monitor support, that seems to be working quite well on my setup. I've included the ability for themes to get metadata about their window, including whether or not they're primary, and have hidden the login components for non-primary screens on the default themes.

I haven't yet implemented cross-window communication, but wanted to get this PR open as a draft before that to make sure that you're happy i'm on the right track. That's the last remaining part, and i'll use the dracula background-switcher as a demo of this functionality.

TODO:

  • Implement cross-window communication
  • Decide on mechanism for themes to pick per-window URLs
  • Fix sending metadata on theme refreshes

Copy link
Owner

@JezerM JezerM left a comment

Choose a reason for hiding this comment

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

Overall, looks nice!

ts/preload.ts Outdated Show resolved Hide resolved
ts/consts.ts Outdated Show resolved Hide resolved
ts/browser.ts Outdated Show resolved Hide resolved
ts/browser.ts Outdated Show resolved Hide resolved
s0 added 7 commits January 15, 2022 23:41
When a window produces an error message,
display the error as a message for the same window.
To aid in easier development workflow,
the theme directory and config path can be overwritten with
environment variables to avoid having to run `node make install`
after changes to themes to test them,
and to allow for a separate configuration file for development and
testing, so that nody-greeter can continue to be used as the
primary greeter on a system with sensible & safe config
Using --no-install ensures that the local node_modules version of
typescript is used, and that an error is thrown if it's not
installed for some reason
Given that this functionality is unique to nody-greeter,
a new class / namespace has been created under `nody_greeter` that
includes the ability to get this information, to allow for the
existing interfaces to remain backwards-compatible with
web-greeter
All other monitors will continue to display the wallpaper
for the theme.
@s0
Copy link
Contributor Author

s0 commented Jan 15, 2022

Thanks for the initial review! Also noticed you introduced linting and formatting, very nice to see! I've incorporated your suggestions in a rebase, and also ensured that it's formatted + linted throughout the history.

I also spotted a bug where if a frame is reloaded (via ctrl+R, or via theme reloading etc...), then it won't initialize as metadata isn't sent more than once, so i'm going to look at fixing that now by having the theme frame request the metadata and sending it then, tracked this in a TODO above.

s0 added 3 commits January 16, 2022 00:15
metadata would only be sent once, and not after page refreshes or
after reloading the window. To address this, the metadata is
caculated once and stored in the windows array, and is requested
by the preload script for every window load.
Using the broadcast mechanism ensures that the wallpaper is
changed across all screens.
@s0 s0 marked this pull request as ready for review January 16, 2022 00:45
@s0
Copy link
Contributor Author

s0 commented Jan 16, 2022

Okay I've now updated the PR addressing the refresh bug, and allowing cross-window communication via a broadcast mechanism. This mechanism is then also used in the dracula theme to apply wallpaper changes across all screens.

Other than the open question around handling of a secondary URL, i think this is ready to go now :)

@JezerM
Copy link
Owner

JezerM commented Jan 16, 2022

Now, nody-greeter will look for index.theme (still not sure about the name) inside the theme's directory, if it does exist then it will be loaded and used to set primary_url and secondary_url for all monitors. However, if the secondary_html property does not exist, it will fallback to the primary_html.

@JezerM
Copy link
Owner

JezerM commented Jan 16, 2022

I just added the secondary.html and index.theme for both gruvbox and dracula themes.

Added a "fake" display to test it locally, and yeah, it works!

vokoscreenNG-2022-01-15_22-39-26.mov

Copy link
Contributor Author

@s0 s0 left a comment

Choose a reason for hiding this comment

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

Cool, it will be useful functionality to have a theme config! :-D

Now, nody-greeter will look for index.theme (still not sure about the name)

Hmm, i'm thinking it should probably end in .yml so that editors will at least pick up the correct syntax-highlighting for people, maybe theme.yml or manifest.yml?

ts/config.ts Outdated Show resolved Hide resolved
ts/config.ts Outdated Show resolved Hide resolved
ts/config.ts Outdated Show resolved Hide resolved

if (theme_dir.endsWith(".html")) theme_dir = path.dirname(theme_dir);

if (!fs.existsSync(theme_dir)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just realised thata we're using synchronous / blocking filesystem operations throughout here. It would be better to use async functions, and use await fs.promises, so that we don't freeze the Node.js process.

ts/config.ts Outdated Show resolved Hide resolved
ts/config.ts Outdated Show resolved Hide resolved
@s0 s0 mentioned this pull request Jan 16, 2022
11 tasks
@JezerM
Copy link
Owner

JezerM commented Jan 17, 2022

io-ts looks interesting, powerful and easy to use. I did a little implementation with both greeter and theme's config, but I'd like you to check if this is correct or there's any improvement to be made.

Copy link
Contributor Author

@s0 s0 left a comment

Choose a reason for hiding this comment

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

Looking good, i think the code can be cleaned up a little more though. It's a bit hard to explain thoroughly with review comments so i'll push a commit in a bit that does what i'm thinking

ts/config.ts Outdated Show resolved Hide resolved
ts/config.ts Outdated Show resolved Hide resolved
@s0
Copy link
Contributor Author

s0 commented Jan 17, 2022

Just my one comment regarding undefined above probably needs to be addressed, then i think this is ready to go! :) Happy to defer the conversation around sync / async I/O operations to #13

@JezerM
Copy link
Owner

JezerM commented Jan 17, 2022

Nice~ It looks better and simpler. Now, I'm going to check if there's any issue or anything to be improved before I merge this PR.

@s0
Copy link
Contributor Author

s0 commented Jan 17, 2022

Nice~ It looks better and simpler. Now, I'm going to check if there's any issue or anything to be improved before I merge this PR.

Sweet! Excited to get this in! And thanks for being an easy maintainer to work with!

Copy link
Owner

@JezerM JezerM left a comment

Choose a reason for hiding this comment

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

I guess everything is alright! Thanks for your work!

Additionally, I just wanted to point out that if this feature is ever going to be added to web-greeter, a few things here could be changed (hopefully, just some nomenclature) to make them compatible~

@JezerM JezerM merged commit ded587a into JezerM:master Jan 17, 2022
mshernandez5 added a commit to mshernandez5/WelcomeXP that referenced this pull request Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-Monitor Support
2 participants