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

Refactor main.js, enable custom screensize #1794

Closed
wants to merge 2 commits into from

Conversation

pciavald
Copy link
Contributor

@pciavald pciavald commented May 12, 2021

I've extracted most of the electron-related logic into electron.js and grouped together behaviors that were a bit scattered in main.js. The logic is more state based and with the default values for BrowserWindow and state overrides, it was easy to implement custom window properties from the user config.

As an example, we use it with these settings:

{
  "config": {
    octodash": {
      "window": {
        "width": 800,
        "height": 454,
        "y": 26,
        "fullscreen": false
      }
    }
  }
}

x is also exposed and was not included in this example to show how only needed values can be passed.

If you prefer that these are written by default in the config file, i can do it based on what's currently happening. Feel free to close if you don't want it in upstream.

Fixes #1586
Supplements #1627

@@ -693,9 +692,9 @@
}
},
"node_modules/@babel/compat-data": {
"version": "7.13.15",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs to be regenerated on your side

Copy link
Owner

@UnchartedBull UnchartedBull left a comment

Choose a reason for hiding this comment

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

Why was the protocol removed? Was this tested on an actual machine or just in a dev environment?

Please make the newly added config entries required and provide the default values in app.service.ts

height: null,
x: null,
y: null,
fullscreen: null,
Copy link
Owner

Choose a reason for hiding this comment

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

fullscreen should default to true

Comment on lines +130 to +133
width: null,
height: null,
x: null,
y: null,
Copy link
Owner

Choose a reason for hiding this comment

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

numbers would make more sense here i guess.

Copy link
Owner

Choose a reason for hiding this comment

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

defaults should be: 800, 480, 0, 0, since that is the main resolution OctoDash is being developed for.

x: null,
y: null,
fullscreen: null,
backgroundColor: null,
Copy link
Owner

Choose a reason for hiding this comment

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

please use the OctoDash background-color here (or black)

Comment on lines +24 to +26
function readConfig(window) {
const config = fetchConfig()
if (config) {
Copy link
Owner

Choose a reason for hiding this comment

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

I guess it would make sense to merge fetchConfig and readConfig.

Comment on lines +34 to +41
frame: false,
fullscreen: true,
useContentSize: true,
width: mainScreen.size.width,
height: mainScreen.size.height,
x: 0,
y: 0,
backgroundColor: '#353b48',
Copy link
Owner

Choose a reason for hiding this comment

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

this part should be loaded from the config

Comment on lines +78 to +85
const config = fetchConfig();
if (config.octodash.window) {
replaceExistingProperties(
properties.window,
config.octodash.window,
['width', 'height', 'x', 'y', 'fullscreen', 'backgroundColor'],
);
}
Copy link
Owner

Choose a reason for hiding this comment

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

this can be removed if the config properties are required.

Copy link
Owner

Choose a reason for hiding this comment

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

Just had another look here, only thing that should be done here is that width, height, x, y should be set to mainScreen.size.width, mainScreen.size.height, 0, 0 if fullscreen is true. Otherwise nothing should be done here.

@pciavald
Copy link
Contributor Author

pciavald commented May 24, 2021

Why was the protocol removed? Was this tested on an actual machine or just in a dev environment?

The protocol was removed because it was not used when using full path for languages instead of the initial virtual path that can't be used with i18n. Tested in dev and prod for more than a month

@stale
Copy link

stale bot commented Jun 7, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issue label Jun 7, 2021
@pciavald pciavald removed the stale Stale issue label Jun 7, 2021
@stale
Copy link

stale bot commented Jun 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issue label Jun 22, 2021
@UnchartedBull UnchartedBull added enhancement New feature or request and removed stale Stale issue labels Jun 23, 2021
@pciavald
Copy link
Contributor Author

I am sorry to announce that I do not have the work power to maintain all our changes in compliance with upstream requirements. This feature has been published to our repo CosmoDash.

@pciavald pciavald closed this Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure window size and position
2 participants