Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Implement Windows Explorer "Open with..." option (no .msi) #299

Merged

Conversation

bchintx
Copy link
Contributor

@bchintx bchintx commented Aug 14, 2013

Overview

This change implements the Edge Code and Brackets user stories that allow Brackets to support the Windows Explorer "Open With..." command.

Please note that we still need an installer change to (1) pre-populate the necessary Registry entries prior to first launch; and (2) clean up the ProgID entries on Brackets uninstallation. However, even without these installer changes, the "Open With..." feature will still work, so long as you launch Brackets at least once before attempting to "open with..." a file.

Example Workflows

  1. install Brackets (or launch Brackets at least once)
  2. launch Windows Explorer
  3. browse to and select a supported file (eg. a .js file)
  4. right-click and choose "Open With..."
  5. select Brackets.exe from the resulting context menu or dialog (note: depending on whether other applications have also associated themselves with this file type, you might either see a dialog or context menu from which to select Brackets.exe)
  6. one of the following occur, depending on whether Brackets is already running:
    • if Brackets isn't already running, launches Brackets, opens selected file, and adds it to the working set;
    • if exactly one instance of Brackets is already running, opens selected file in that instance of Brackets, and adds it to the working set; or
    • if >1 instance of Brackets is already running, opens selected file in the top-most (ie. most recently used) instance of Brackets, and adds it tot he working set.

[note: Brackets Sprint 28 already supports opening a single file from the Brackets command-line. However, it always launches a new instance to open the given file.]

Implementation

This Windows-only, brackets-shell change adds two, new pieces of functionality:

  1. opens given file in the most-recent running instance, rather than launching a new Brackets window; and
  2. adds initialization/update of file association entries in the Windows Registry

For the first item, I've added a mutex to track when a Brackets instance is already running. Then, when a new file is opened from the command-line, the new instance will enumerate any already running instances of Brackets, starting with the top-most window instance (aka the most-recently used window), bring it to the front of the desktop windows, and send it the filename via a WM_COPYDATA message. Then, the second instance will immediately exit. Upon receipt of the WM_COPYDATA message, the receiving instance of Brackets will then call existing Javascript code to open the file and add it to the working set.

For the second item, I've added initialization code to create/update Registry entries corresponding to those associated with an application ProgID and individual Explorer FileExts entries in support of the "Open With..." command. This extra measure of initialization/update will help keep Brackets up-to-date, if for example you are running different builds of Brackets, without requiring that you uninstall and reinstall the application in order to continue using the "Open With..." command.

Note: this pull request replaces PR #296, in which I accidentally committed .msi to the branch.

… Windows Explorer 'Open With...' menu and dialog for supported file types
@bchintx
Copy link
Contributor Author

bchintx commented Aug 14, 2013

Submitted two changes to address two issues found by David Alcala.

// open an existing file on the command-line (eg. Open With.. from Windows Explorer)
HWND hFirstInstanceWnd = NULL;
::EnumWindows(FindFirstBracketsInstance, (LPARAM)&hFirstInstanceWnd);
ASSERT(hFirstInstanceWnd != NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should handle this case by bailing and just creating a new instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

and by bailing I mean that this code should be if (hFirstInstancnceWnd != NULL) { bring to front, send wm_copydata, return 0 } so we can fall through, create the mutex etc if it fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Good call.

@JeffryBooher
Copy link
Contributor

Done with the initial review. There are a few gotchas and some minor nits here that prevent merging. The one big thing that I'd like to see changed is the if block in cefclient_win.cpp:201. I think if we can change that function so that it checks to see if there is an instance then try to use it and fall back to creating a new instance if that fails we'll be in better shape. I'd rather have a new instance created inexplicably due to failure than nothing happen.

@bchintx
Copy link
Contributor Author

bchintx commented Aug 14, 2013

@JeffryBooher Thanks for the review. I've updated the pull request with all of your requested changes, except for the request to skip an already running instance if a Brackets JS-modal dialog is open (see inline comments for my reasoning there).

This is now ready for review again. Thanks!

// open an existing file on the command-line (eg. Open With.. from Windows Explorer)
HWND hFirstInstanceWnd = NULL;
::EnumWindows(FindSuitableBracketsInstance, (LPARAM)&hFirstInstanceWnd);
if (hFirstInstanceWnd != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the indentation got messed up here.

@JeffryBooher
Copy link
Contributor

Just the 2 minor nits and I think this is good to go.

I think I discovered an interesting side effect if both brackets and edge code are installed in that if we don't use a unique mutex name for each app then the Open With command could open the file in the wrong app. I did recommend having the product name in the first pass just thinking it should have a better name than "FirstShellInstance" but didn't think about the use case if both had been installed. Just wondering if you were trying to avoid changing the mutex name in edge code and that's why you went with a generic name.

@bchintx
Copy link
Contributor Author

bchintx commented Aug 15, 2013

@JeffryBooher Just pushed updated code to re-indent using VS2010 and moving APP_DATA into the #define to improve readability. Ready for final review. Thanks!

@bchintx
Copy link
Contributor Author

bchintx commented Aug 15, 2013

@JeffryBooher oh, and just to echo an inline comment that I made (which might have gotten hidden once I pushed my refactored change): the mutex names were actually different between Brackets and EC. I was pre-pending APP_NAME in the two places that I was using the other #define mutex constant. In doing so, each app would end up with a different mutex name since the APP_NAME constant itself was being defined different in each product. Long story short, I moved the APP_NAME pre-pending directly into the mutex name's #define to make it more readable.

@JeffryBooher
Copy link
Contributor

Looks good... Merging

JeffryBooher added a commit that referenced this pull request Aug 15, 2013
…ows-explorer-no-msi

Implement Windows Explorer "Open with..." option (no .msi)
@JeffryBooher JeffryBooher merged commit 1407457 into master Aug 15, 2013
@JeffryBooher JeffryBooher deleted the bchin/open-with-brackets-from-windows-explorer-no-msi branch August 15, 2013 19:47
JeffryBooher added a commit that referenced this pull request Dec 8, 2015
…ows-explorer-no-msi

Implement Windows Explorer "Open with..." option (no .msi)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants