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

Update "File > Open Folder..." dialog with Windows' Open File dialog in pick folders mode. #247

Merged
merged 4 commits into from
May 26, 2013

Conversation

bchintx
Copy link
Contributor

@bchintx bchintx commented May 15, 2013

While investigating 'brackets' issue adobe/brackets#889, which was opened in lieu of 'brackets-app' issue adobe/brackets-app#120, I noticed that (in the latter issue's discussion) NJ suggested looking at newer implementations of the Windows common file dialogs to solve the double-click issue. Unfortunately, it appears that dbl-click, even in the newer implementation, is still not the Microsoft-supported/recommended way of selecting and confirming the dialog. However, here's code to update the dialog, in case we're interested.

This Pull Request replaces the code implementing Brackets' feature "File > Open Folder..." currently using SHBrowseForFolder() with the MSDN-recommended implementation using IFileDialog + FOS_PICFOLDERS.

More specifically, as suggested by MSDN (http://msdn.microsoft.com/en-us/library/windows/desktop/bb762115%28v=vs.85%29.aspx), for Vista and later OSes, apps should use the newer Open File dialog in pick folders mode.

The only "downside" to pulling this new code (so far as I can tell) is that the selected folder is remembered for the next time that the dialog appears, which might interfere with the "Please select the Brackets index.html file" dialog in development builds.

@bchintx
Copy link
Contributor Author

bchintx commented May 15, 2013

FYI... no hurt feelings if y'all decide not to take this change. I did it mostly to see if it'd fix the aforementioned 'brackets-app' issue. The new dialog drops in quite nicely and gives a more full-featured dialog with a UI that's more consistent with the Mac implementation. However, since it doesn't solve Peter's original issue, I'm not sure it's a must-have change.

If y'all prefer to leave the current implementation, we can just close this pull request.

@peterflynn
Copy link
Member

@bchintx Brackets supports Windows XP too, so we'd have to keep the old code as a fallback codepath depending on OS version.

@redmunds
Copy link
Contributor

@bchintx This is definitely better. As @peterflynn said, we still have to support WinXP, but eventually we want to use the new dialog. Can you add a conditional to check to WinXP and use the old dialog, otherwise use the new dialog? That way your code won't get lost and we jus need to remove the old code when we drop support for WinXP.

@bchintx
Copy link
Contributor Author

bchintx commented May 16, 2013

Great observation, @peterflynn, and suggestion @redmunds. I've added a check to call the old, folder select dialog if the current OS is pre-Vista (eg. XP) or the new, IFileFolder dialog if the current OS is Vista or later.

@bchintx
Copy link
Contributor Author

bchintx commented May 16, 2013

FYI- I noticed that this code change also addresses this Trello card, currently on the Icebox:
https://trello.com/card/windows-use-more-modern-open-dialog-on-vista-win-7/4f90a6d98f77505d7940ce88/417

@ghost ghost assigned redmunds May 17, 2013
@redmunds
Copy link
Contributor

Please remove this line of code in appshell_extensions_win.cpp line 418:

    // TODO (issue #64) - This method should be using IFileDialog instead of the

@redmunds
Copy link
Contributor

@bchintx I saw this comment you made above:

The only "downside" to pulling this new code (so far as I can tell) is that the selected folder
is remembered for the next time that the dialog appears, which might interfere with the
"Please select the Brackets index.html file" dialog in development builds.

This seems to behave the same as Sprint 24. Are you aware of the setup_for_hacking.sh script so you don't get prompted for Brackets index.html on every startup in a dev build?

@redmunds
Copy link
Contributor

@peterflynn Can you confirm that this works correctly on WinXP?

@bchintx
Copy link
Contributor Author

bchintx commented May 17, 2013

@redmunds Yes, I use that script to set the index.html. With that location fixed, the fact that the Open File dialog opens to a different location doesn't really matter anymore. Still, I just wanted to mention it as the only other change in behavior that I could find.

@redmunds
Copy link
Contributor

Thanks, Brian! Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants