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

Restrict extension to the primary monitor #109

Merged
merged 2 commits into from
May 4, 2019
Merged

Conversation

vltr
Copy link
Contributor

@vltr vltr commented May 1, 2019

This PR basically adds the option to restrict the overall extension functionality to the primary screen (if enabled, default) or all screens (if disabled). This affects the window decorations, window buttons and application menu.

@jonian
Copy link
Member

jonian commented May 2, 2019

Thank you for your contribution. Maybe you can get restrict-to-primary-screen setting in the helper function to avoid repetition. In the helper file you can get the setting with:

const Settings = Unite.imports.convenience.getSettings();

Settings.getSetting('restrict-to-primary-screen')

@vltr
Copy link
Contributor Author

vltr commented May 3, 2019

Thanks, @jonian ! I was in doubt if having a setting getter inside helpers.js would be a good idea - hence why I made this way, but since I agree with you it would be better to avoid repetition.

Should I leave the signals on their respective modules? (I guess they should because they may have to work based on its value).

@jonian
Copy link
Member

jonian commented May 4, 2019

@vltr Sorry for the late response. Yes, you should leave the signals on their respective modules. They are used to apply changes without restarting the shell.

@vltr
Copy link
Contributor Author

vltr commented May 4, 2019

@jonian don't worry 😉

They are used to apply changes without restarting the shell.

That was exactly my thought. I'll update this PR shortly after.

…r function to take only two arguments (as it was previously)
@vltr
Copy link
Contributor Author

vltr commented May 4, 2019

Done. I had made this change in my computer yesterday already and the extension is running flawlessly (so far) 💪

Copy link
Member

@jonian jonian left a comment

Choose a reason for hiding this comment

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

Great! Thank you for contributing.

@jonian jonian merged commit c6ffe53 into hardpixel:master May 4, 2019
@jonian
Copy link
Member

jonian commented May 4, 2019

Fixes issue #61

@vltr
Copy link
Contributor Author

vltr commented May 4, 2019

Great! Thank you for contributing.

It was my pleasure 😉 Thanks a lot!

@vltr
Copy link
Contributor Author

vltr commented May 4, 2019

Oh, @jonian , I did not change any metadata or the README file because I guess the maintainers would like to do that when the next release comes to light 😃

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.

2 participants