Open in External Editor, But Simpler™ #2407

shiftkey · 2017-08-06T21:43:46Z

This is my proposed implementation of #2009, and should be simpler than #2039.

First Pass: Make It Work

lookup to see if the user has each program installed
- Atom
- VSCode
- Sublime Text
Preferences dialog listing available editors and
Persist this setting somewhere when it changes (localStorage?)
Top-level menu item for "Open in External Editor"
- ensure this actually launches the selected editor

This should be enough to confirm we're on the right track.

Second Pass - polishing things

top-level menu should show the editor name here as well
Repository menu item for "Open in External Editor"
performance - this lookup could be expensive - cache it for the session
macOS should invoke shims instead of launching app
docs about how this integration works
on first launch, set a default editor - awaiting to lookup of all editors adds +1s on Windows to AppStore.loadInitialState when it fails - maybe i shouldn't do this
when the editor doesn't exist, you need to go in to Preferences and manually switch to a different editor (the first one looks like it's selected but lol no) for it to actually update. that's bananas.
merge in open Preferences/Options to a given tab #2413 and get "Open Preferences/Options" action to focus on Advanced tab
find a faster registry implementation for Windows

Screens and Visuals

With editors found:

Preferences view

With no editors found:

defaults to Open in Atom - should this be 'Open in External Editor' instead?
Preferences view should show "Go install Atom" link instead of select

error dialog when you don't have any editors suggests you install Atom

error dialog when the editor can't be found, and you have to go to preferences and try again

Assorted other notes:

app-path [repo] uses a precompiled binary. I'd rather that use node-gyp from a purist perspective, but we can PR that later unless we have strong feels.

j-f1 · 2017-08-06T23:44:33Z

It might be good to use something like electron-settings to save the preferences to a file that users could potentially edit.

shiftkey · 2017-08-07T00:10:26Z

It might be good to use something like electron-settings to save the preferences to a file that users could potentially edit.

We don't do this for any of our current preferences, so I'm going to defer this to another discussion.

app/src/main-process/main.ts

  createWindow()

-  const menu = buildDefaultMenu()
+  let menu = buildDefaultMenu()


brandonzylstra · 2022-12-02T22:58:13Z

app/src/main-process/menu/build-default-menu.ts

+      {
+        label: `Open in ${editorLabel}`,
+        id: 'open-external-editor',
+        accelerator: 'CmdOrCtrl+Shift+A',


CmdOrCtrl+Shift+A? Is that really the conventional keystroke shortcut for that command? I seem to remember that command using an E in other apps where I've seen it (though I don't recall the modifier keys).

app/src/lib/editors/darwin.ts

+
+  try {
+    // invoked without assigning the result so we verify the app is installed
+    await appPath('com.github.atom')


app/src/lib/editors/darwin.ts

+      'Contents',
+      'SharedSupport',
+      'bin',
+      'subl'


app/src/lib/editors/win32.ts

+    const regKey = new Registry({
+      hive: Registry.HKLM,
+      key:
+        '\\SOFTWARE\\WOW6432Node\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\{F8A2A208-72B3-4D61-95FC-8A65D340689B}_is1',


shiftkey · 2017-08-07T21:42:51Z

While I go through and tidy up the UX corner cases, this is likely ready for some early 👀

app/src/lib/editors/shared.ts

+ */
+export function pathExists(path: string): Promise<boolean> {
+  return new Promise<boolean>((resolve, reject) => {
+    Fs.exists(path, exists => {


app/src/lib/editors/darwin.ts

+async function findCodeApplication(): Promise<LookupResult> {
+  const name = VisualStudioCodeLabel
+  try {
+    const installPath = await appPath('com.github.atom')


joshaber · 2017-08-08T14:57:32Z

This seems reasonable so far 👍 I was a little unsure of using the apps' command line launchers, vs. launching the app itself, but after thinking about it I suppose that's probably the better supported launching method anyways.

shiftkey · 2017-08-08T21:44:41Z

I was a little unsure of using the apps' command line launchers, vs. launching the app itself

As in "launching the .app or .exe directly"?

but after thinking about it I suppose that's probably the better supported launching method anyways.

Yeah, these are full-fledged interfaces that are copied over to /usr/local/bin on macOS. My initial testing around this lazily would just invoke the .exe - it took me a couple of attempts to remember that this wasn't right.

$ atom --help
Atom Editor v1.18.0

Usage: atom [options] [path ...]

One or more paths to files or folders may be specified. If there is an
existing Atom window that contains all of the given folders, the paths
will be opened in that window. Otherwise, they will be opened in a new
window.

... 

$ code --help
Visual Studio Code 1.14.2

Usage: code [options] [paths...]

Options:
...

And as Atom points out, the handling of "open a new window" versus "focus existing window" should be handled by the app through these interfaces.

shiftkey · 2017-08-09T00:19:07Z

Some notes after profiling this PR:

Windows

No longer using winreg as I can extract the magic and make this code fast:

when an application cannot be found in the registry, failure is reported quickly (~30ms)
when an application is found, the registry information is returned to the caller in ~40ms

macOS

the result reported from either success or failure is about the same (~25ms)
the overall time to resolve all editors is approx ~37ms

donokuda · 2017-08-11T00:57:49Z

Done in cf4514a

Tweak styles for no editors found message

…e has a 64-bit build

shiftkey · 2017-08-11T02:44:49Z

I might have missed it, but I think we also need to disable the menu item if the repository is missing on disk.

This should be covered by the repository menu being disabled, but 4b8407e makes this more explicit.

shiftkey · 2017-08-11T04:49:33Z

🌔

joshaber

Just needs a merge now and this'll be good to go 🔜

Really nice work on this 💖

app/src/lib/editors/darwin.ts

+    case ExternalEditor.SublimeText:
+      return 'com.sublimetext.3'
+    default:
+      return assertNever(editor, `Unknown external editor: ${editor}`)


shiftkey · 2017-08-13T05:15:18Z

🍎

joshaber · 2017-08-14T14:48:28Z

🕺

app/src/lib/editors/darwin.ts

+      'app',
+      'atom.sh'
+    )
+    const exists = await pathExists(path)


app/src/lib/editors/registry.ts

+  value: string
+}
+
+const regPath = 'C:\\Windows\\System32\\reg.exe'


zeke · 2017-08-15T04:23:27Z

👍

shiftkey force-pushed the open-in-external-editor branch from d4f680a to bd5c93c Compare August 6, 2017 23:06

shiftkey commented Aug 7, 2017

View reviewed changes

app/src/main-process/main.ts

createWindow()

const menu = buildDefaultMenu()

let menu = buildDefaultMenu()

This comment was marked as spam.

Sign in to view

shiftkey commented Aug 7, 2017

View reviewed changes

app/src/lib/editors/darwin.ts Outdated

try {

// invoked without assigning the result so we verify the app is installed

await appPath('com.github.atom')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

shiftkey commented Aug 7, 2017

View reviewed changes

app/src/lib/editors/darwin.ts Outdated

'Contents',

'SharedSupport',

'bin',

'subl'

This comment was marked as spam.

Sign in to view

shiftkey commented Aug 7, 2017

View reviewed changes

app/src/lib/editors/win32.ts Outdated

const regKey = new Registry({

hive: Registry.HKLM,

key:

'\\SOFTWARE\\WOW6432Node\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\{F8A2A208-72B3-4D61-95FC-8A65D340689B}_is1',

This comment was marked as spam.

Sign in to view

shiftkey mentioned this pull request Aug 7, 2017

Open Repository in External Editor #2009

Closed

shiftkey mentioned this pull request Aug 8, 2017

open Preferences/Options to a given tab #2413

Merged

Janpot reviewed Aug 8, 2017

View reviewed changes

app/src/lib/editors/shared.ts Outdated

*/

export function pathExists(path: string): Promise<boolean> {

return new Promise<boolean>((resolve, reject) => {

Fs.exists(path, exists => {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

shiftkey mentioned this pull request Aug 8, 2017

fs.exists is obsolete #2415

Closed

joshaber self-assigned this Aug 8, 2017

joshaber reviewed Aug 8, 2017

View reviewed changes

app/src/lib/editors/darwin.ts Outdated

async function findCodeApplication(): Promise<LookupResult> {

const name = VisualStudioCodeLabel

try {

const installPath = await appPath('com.github.atom')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

stub out the UI for selecting an editor

4e28f67

shiftkey force-pushed the open-in-external-editor branch from bfec7aa to c8cad9b Compare August 9, 2017 22:55

shiftkey added 10 commits August 10, 2017 10:45

added app-path dependency

759b4d2

add macOS lookup logic using external module and bundle identifiers

972201a

using winreg for interacting with the windows registry

07dc796

implement registry lookups for atom and sublime

b8ae8f2

extract into platform-specific modules for a bit of maintainability

d2f8758

T Y P E S

4f0d1f0

and enable VSCode on Windows

17cd0da

and some more prettify

118c02e

store and update localStorage when the editor is changed

023137b

invoke the CLI interfaces for these tools

552f7fd

Smaller width for error dialog

cf4514a

shiftkey and others added 7 commits August 11, 2017 11:03

Merge pull request #2448 from desktop/open-in-editor-pref-polish

8127fd7

Tweak styles for no editors found message

add support for looking up multiple registry locations now that VSCod…

ff97685

…e has a 64-bit build

move updateExternalEditorMenuItem to main-process-proxy

e4d7bff

fewer magic strings

88835df

classic macOS

2eec847

actually this is the right casing

fe64844

ensure menu item is disabled when repository is missing

4b8407e

shiftkey added 3 commits August 11, 2017 13:12

stay inside the monard

20909e3

invert null check

6437422

Merge branch 'master' into open-in-external-editor

b2e7481

joshaber suggested changes Aug 11, 2017

View reviewed changes

app/src/lib/editors/darwin.ts

case ExternalEditor.SublimeText:

return 'com.sublimetext.3'

default:

return assertNever(editor, `Unknown external editor: ${editor}`)

This comment was marked as spam.

Sign in to view

Merge branch 'master' into open-in-external-editor

7eaa874

joshaber approved these changes Aug 14, 2017

View reviewed changes

joshaber merged commit d9ae817 into master Aug 14, 2017

joshaber deleted the open-in-external-editor branch August 14, 2017 14:48

This was referenced Aug 15, 2017

Open with external editor runs batch files on windows #2464

Closed

Add support for finding Atom, Visual Studio, and VS Code #2039

Closed

shimpossible reviewed Aug 15, 2017

View reviewed changes

app/src/lib/editors/darwin.ts Outdated

'app',

'atom.sh'

)

const exists = await pathExists(path)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

shimpossible reviewed Aug 15, 2017

View reviewed changes

app/src/lib/editors/registry.ts Outdated

value: string

}

const regPath = 'C:\\Windows\\System32\\reg.exe'

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

shiftkey mentioned this pull request Aug 15, 2017

use %WINDIR% environment variable instead of hard-coding #2466

Merged

bbgdzxng1 mentioned this pull request Aug 20, 2017

Open in Github Desktop from External Editor #2509

Closed

shiftkey mentioned this pull request Sep 20, 2017

don't crash the app on non-macOS and non-Windows platforms #2807

Merged

niik mentioned this pull request Jan 16, 2019

Menu state race condition #6643

Merged

Open in External Editor, But Simpler™ #2407

Open in External Editor, But Simpler™ #2407

Uh oh!

Conversation

shiftkey commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First Pass: Make It Work

Second Pass - polishing things

Screens and Visuals

Uh oh!

j-f1 commented Aug 6, 2017

Uh oh!

shiftkey commented Aug 7, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

brandonzylstra Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

shiftkey commented Aug 7, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

joshaber commented Aug 8, 2017

Uh oh!

shiftkey commented Aug 8, 2017

Uh oh!

shiftkey commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Windows

macOS

Uh oh!

donokuda commented Aug 11, 2017

Uh oh!

shiftkey commented Aug 11, 2017

Uh oh!

shiftkey commented Aug 11, 2017

Uh oh!

joshaber left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

shiftkey commented Aug 13, 2017

Uh oh!

joshaber commented Aug 14, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

zeke commented Aug 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

shiftkey commented Aug 6, 2017 •

edited

Loading

shiftkey commented Aug 9, 2017 •

edited

Loading