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

🎉 Check for WScript.exe #1872

Merged
merged 7 commits into from
May 26, 2017
Merged

🎉 Check for WScript.exe #1872

merged 7 commits into from
May 26, 2017

Conversation

albinekb
Copy link
Contributor

@albinekb albinekb commented May 26, 2017

This is my proposed solution for handling #1812

We could add a key in the config for setting the editor, but I think that's a bad idea. Better to teach the users to set the correct app for .js files :) maybe put an alert/dialog with a link on how to do it.

At least this will let people open their config without errors, and we can think about how we can make this better in the future

Fixes #1812 #1810 #1784 (comment) #1854

@albinekb albinekb added the 💬 Feedback Wanted Issue or PR needs input from the community! Lend your thoughts ✨ label May 26, 2017
@albinekb albinekb force-pushed the notepad branch 2 times, most recently from c78f034 to 2455c78 Compare May 26, 2017 07:30
@@ -52,7 +51,7 @@ module.exports = function () {
label: 'Preferences...',
accelerator: accelerators.preferences,
click() {
shell.openItem(confPath);
openConfig();
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep fixed @chabou 👌

@chabou
Copy link
Contributor

chabou commented May 26, 2017

I really love this solution! ❤️

@albinekb albinekb added the 💛 Priority: Medium Issue isn't of the highest priority but still important label May 26, 2017
@ppot ppot merged commit 07ef007 into master May 26, 2017
@albinekb albinekb deleted the notepad branch May 26, 2017 16:23
@albinekb albinekb mentioned this pull request May 26, 2017
2 tasks
const {shell} = require('electron');
const {confPath} = require('./paths');

module.exports = () => Promise.resolve(shell.openItem(confPath));

Choose a reason for hiding this comment

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

Why are there 2 module.exports in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we want to have a different module on windows compared to others

if (error) {
reject(error);
} else if (stdout && stdout.includes('WScript.exe')) {
reject(new Error('WScript is the default editor for .js files'));

Choose a reason for hiding this comment

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

If WScript.exe exists, this will throw an error, which will then open the conf file with notepad? I thought the idea is to use WScript.exe if it exists?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tnguyen14 No, WScript.exe run the file instead of editing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will check if your default editor is set to WScript.exe, if it is , it will use notepad.exe, since WScript.exe isn't an editor

Choose a reason for hiding this comment

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

Ah. I see. I understand now. Thank you for the clarification. I was confused by this comment: Windows opens .js files with WScript.exe by default

@Stanzilla
Copy link
Contributor

Does not work on Windows, even though a default program other than WScript is set for .js, it opens notepad.

@albinekb albinekb mentioned this pull request Jun 9, 2017
@dave-irvine
Copy link

I've got a bit of a problem here in that you can manually change filetype associations like this:

ftype JSFile=notepad.exe %1 %*

but then hyper won't start any more. So without a configuration option to change what editor opens the configuration file, I'm stuck with notepad.

Any suggestions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 Feedback Wanted Issue or PR needs input from the community! Lend your thoughts ✨ 💛 Priority: Medium Issue isn't of the highest priority but still important
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants