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

Fix for issue #6084: Move Find items to different menu #7488

Merged
merged 10 commits into from Apr 24, 2014

Conversation

Projects
None yet
9 participants
@lkcampbell
Contributor

lkcampbell commented Apr 11, 2014

Fix for issue #6084: Move Find items to different menu.

Creates a new Search Find menu.

Moves appropriate Edit menu items over to new Search Find menu.

Adds a new Search Find menu item "Find in Selected File/Folder". It is connected to the same command as the Project panel context menu "Find in..." menu item.

@peterflynn

View changes

Show outdated Hide outdated src/command/Commands.js
@njx

View changes

Show outdated Hide outdated src/nls/root/strings.js
@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Apr 15, 2014

Member

Review done, just a couple of comments. The more I think about it the more I think we should rename it to "Find" - @JeffryBooher had raised the concern that these are usually in the Edit menu in Windows apps, and so moving them to another menu might impede discoverability, but if we name the menu Find I think it will be very obvious where those menu items are :)

Member

njx commented Apr 15, 2014

Review done, just a couple of comments. The more I think about it the more I think we should rename it to "Find" - @JeffryBooher had raised the concern that these are usually in the Edit menu in Windows apps, and so moving them to another menu might impede discoverability, but if we name the menu Find I think it will be very obvious where those menu items are :)

@larz0

This comment has been minimized.

Show comment
Hide comment
@larz0

larz0 Apr 15, 2014

Member

+1 for "Find"

Member

larz0 commented Apr 15, 2014

+1 for "Find"

@JeffryBooher

This comment has been minimized.

Show comment
Hide comment
@JeffryBooher

JeffryBooher Apr 15, 2014

Contributor

Mostly Muscle Memory. For many years I didn't use Ctrl+F or Ctrl+H to search/replace I would Alt+E>F or Alt+E>R. I may be the only one in the world who did that or maybe not. I still find myself doing it on occasion plus that's the place where most folks look. It's also in the Windows Style Guide for Application Interface design.

Contributor

JeffryBooher commented Apr 15, 2014

Mostly Muscle Memory. For many years I didn't use Ctrl+F or Ctrl+H to search/replace I would Alt+E>F or Alt+E>R. I may be the only one in the world who did that or maybe not. I still find myself doing it on occasion plus that's the place where most folks look. It's also in the Windows Style Guide for Application Interface design.

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Apr 15, 2014

Member

I took a look at a few apps on Windows. It looks like most modern Office apps no longer have an Edit menu, although they do have special support for the Alt+E shortcuts (a little toast pops up saying something like "Continue typing the menu key sequence from an earlier version of Office"). I also checked Notepad++, which I think is one of the most popular text editors on Windows, and they also have all the find items in a separate Search menu (and it doesn't support Alt+E, F and the like). Looking at the screenshots of Windows editors on http://sixrevisions.com/web-development/the-15-most-popular-text-editors-for-developers/, it looks like many if not most of those apps have a separate Search menu too. So I think we're on ok ground here.

Member

njx commented Apr 15, 2014

I took a look at a few apps on Windows. It looks like most modern Office apps no longer have an Edit menu, although they do have special support for the Alt+E shortcuts (a little toast pops up saying something like "Continue typing the menu key sequence from an earlier version of Office"). I also checked Notepad++, which I think is one of the most popular text editors on Windows, and they also have all the find items in a separate Search menu (and it doesn't support Alt+E, F and the like). Looking at the screenshots of Windows editors on http://sixrevisions.com/web-development/the-15-most-popular-text-editors-for-developers/, it looks like many if not most of those apps have a separate Search menu too. So I think we're on ok ground here.

@JeffryBooher

This comment has been minimized.

Show comment
Hide comment
@JeffryBooher

JeffryBooher Apr 15, 2014

Contributor

I think it's fine. Our Edit menu is getting quite large so this makes for a logical split. FWIW -- Visual Studio and Notepad carry on the tradition of Edit > Find in the modern age...

Contributor

JeffryBooher commented Apr 15, 2014

I think it's fine. Our Edit menu is getting quite large so this makes for a logical split. FWIW -- Visual Studio and Notepad carry on the tradition of Edit > Find in the modern age...

@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 15, 2014

Contributor

@njx, reviewed your review. I have a few follow up questions posted above.

Contributor

lkcampbell commented Apr 15, 2014

@njx, reviewed your review. I have a few follow up questions posted above.

@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 16, 2014

Contributor

@njx or @TomMalbran, code review changes committed.

The deprecation code should prevent the three affected extensions from blowing up.

I am going to post issues for the extension authors as well.

Contributor

lkcampbell commented Apr 16, 2014

@njx or @TomMalbran, code review changes committed.

The deprecation code should prevent the three affected extensions from blowing up.

I am going to post issues for the extension authors as well.

@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 16, 2014

Contributor

@TomMalbran, I saw your brief comment and it brings up an interesting point I want to discuss.

I can understand how to show a deprecation warning in an invoked function. You just throw the warning in the body of the function. I am less clear on how to show a deprecation warning when a deprecated data member is accessed. Maybe the new Getter functions?

At any rate, my solution was to look at how the three extension authors used the constants and, in two cases, put my deprecation warning in the member function, addMenuItem(), that they were invoking. In the third case, the extension is assigning the constant to an object property:

{
  id:         ISEARCH_FORWARD,
  name:       "ISearch Forward",
  key:        "Ctrl-S",
  overrideId: Commands.EDIT_FIND
}

Not sure where to put the deprecation warning function in this scenario so I just pointed the old constant values to legitimate new values and posted an issue for the extension author.

Contributor

lkcampbell commented Apr 16, 2014

@TomMalbran, I saw your brief comment and it brings up an interesting point I want to discuss.

I can understand how to show a deprecation warning in an invoked function. You just throw the warning in the body of the function. I am less clear on how to show a deprecation warning when a deprecated data member is accessed. Maybe the new Getter functions?

At any rate, my solution was to look at how the three extension authors used the constants and, in two cases, put my deprecation warning in the member function, addMenuItem(), that they were invoking. In the third case, the extension is assigning the constant to an object property:

{
  id:         ISEARCH_FORWARD,
  name:       "ISearch Forward",
  key:        "Ctrl-S",
  overrideId: Commands.EDIT_FIND
}

Not sure where to put the deprecation warning function in this scenario so I just pointed the old constant values to legitimate new values and posted an issue for the extension author.

@TomMalbran

This comment has been minimized.

Show comment
Hide comment
@TomMalbran

TomMalbran Apr 16, 2014

Contributor

I am think that a getter might be the only way, and would work for all the cases. But we never had to implement it, so not sure what would be the policy about adding it?

Are we going to change more commands constants? The same issue could reappear if we split the Edit menu again in the future.

Contributor

TomMalbran commented Apr 16, 2014

I am think that a getter might be the only way, and would work for all the cases. But we never had to implement it, so not sure what would be the policy about adding it?

Are we going to change more commands constants? The same issue could reappear if we split the Edit menu again in the future.

@lkcampbell lkcampbell referenced this pull request Apr 16, 2014

Closed

Menus get truncated #7496

@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 16, 2014

Contributor

@TomMalbran I agree. At the very least, we are probably going to create a Selection menu pretty soon. The poor Edit menu is getting clobbered with menu items from the native code and extensions. It is having a noticeable effect on the non-scrolling Linux HTML menus: #7496.

It would be nice to have a way to deprecate data members as well and I think getter functions are the only way to do it. I know @njx is not currently available but maybe we can get some opinions from some other core team members: @redmunds @peterflynn.

Contributor

lkcampbell commented Apr 16, 2014

@TomMalbran I agree. At the very least, we are probably going to create a Selection menu pretty soon. The poor Edit menu is getting clobbered with menu items from the native code and extensions. It is having a noticeable effect on the non-scrolling Linux HTML menus: #7496.

It would be nice to have a way to deprecate data members as well and I think getter functions are the only way to do it. I know @njx is not currently available but maybe we can get some opinions from some other core team members: @redmunds @peterflynn.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Apr 16, 2014

Contributor

We're just deprecating values, not a data member, so I think this is ok. If we plan to rename many more, then maybe just add a function to keep the code more isolated.

Contributor

redmunds commented Apr 16, 2014

We're just deprecating values, not a data member, so I think this is ok. If we plan to rename many more, then maybe just add a function to keep the code more isolated.

@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 17, 2014

Contributor

@TomMalbran, my schedule is starting to change this week so I don't think I have the time to take on changing all of the constants. I have already talked to the one guy who is affected by the deprecation of the Edit menu constant values that were moved to the Find menu, so let's just get this PR done for now.

Contributor

lkcampbell commented Apr 17, 2014

@TomMalbran, my schedule is starting to change this week so I don't think I have the time to take on changing all of the constants. I have already talked to the one guy who is affected by the deprecation of the Edit menu constant values that were moved to the Find menu, so let's just get this PR done for now.

@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 17, 2014

Contributor

I filed #7556 to document our discussion of the menu prefix problem.

Contributor

lkcampbell commented Apr 17, 2014

I filed #7556 to document our discussion of the menu prefix problem.

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Apr 21, 2014

Member

FWIW I think you should be able to deprecate the constants using the same trick we're doing for global CodeMirror, using a getter as @TomMalbran suggested: https://github.com/adobe/brackets/blob/master/src/brackets.js#L112

Member

njx commented Apr 21, 2014

FWIW I think you should be able to deprecate the constants using the same trick we're doing for global CodeMirror, using a getter as @TomMalbran suggested: https://github.com/adobe/brackets/blob/master/src/brackets.js#L112

@MarcelGerber

This comment has been minimized.

Show comment
Hide comment
@MarcelGerber

MarcelGerber Apr 21, 2014

Contributor

@lkcampbell @njx I just wrote a getter ready to use and as general as possible:
MarcelGerber@6c6371d

Contributor

MarcelGerber commented Apr 21, 2014

@lkcampbell @njx I just wrote a getter ready to use and as general as possible:
MarcelGerber@6c6371d

@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 22, 2014

Contributor

@njx, ready for the next code review.

Contributor

lkcampbell commented Apr 22, 2014

@njx, ready for the next code review.

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Apr 22, 2014

Member

Cool - I will probably get back to this tomorrow (Wed).

Member

njx commented Apr 22, 2014

Cool - I will probably get back to this tomorrow (Wed).

@MarcelGerber

View changes

Show outdated Hide outdated src/command/Commands.js
@njx

View changes

Show outdated Hide outdated src/command/Menus.js
@@ -463,7 +463,7 @@ define(function (require, exports, module) {
}, "search bar close");
});
runs(function () {
waitsForDone(CommandManager.execute(Commands.EDIT_FIND_IN_FILES));
FindInFiles._doFindInFiles(scope);

This comment has been minimized.

@njx

njx Apr 24, 2014

Member

Not sure why this change was necessary - also, scope isn't defined.

@njx

njx Apr 24, 2014

Member

Not sure why this change was necessary - also, scope isn't defined.

This comment has been minimized.

@lkcampbell

lkcampbell Apr 24, 2014

Contributor

@njx, that was part of fixing the merge conflict I had, see: 73ffa3e

Did I do it correctly?

@lkcampbell

lkcampbell Apr 24, 2014

Contributor

@njx, that was part of fixing the merge conflict I had, see: 73ffa3e

Did I do it correctly?

This comment has been minimized.

@njx

njx Apr 24, 2014

Member

Hmmm, that's strange. In master, the call to openSearchBar() has the scope variable. But in your branch, it doesn't, and the diff here doesn't show this as a diff from master. If I do a test merge of your branch into master, it looks fine, but it's strange that it didn't get merged correctly into your branch. Maybe that other line was part of the conflict and you resolved it incorrectly? Anyway, I'll go ahead and merge this and just double-check that everything looks okay afterwards.

@njx

njx Apr 24, 2014

Member

Hmmm, that's strange. In master, the call to openSearchBar() has the scope variable. But in your branch, it doesn't, and the diff here doesn't show this as a diff from master. If I do a test merge of your branch into master, it looks fine, but it's strange that it didn't get merged correctly into your branch. Maybe that other line was part of the conflict and you resolved it incorrectly? Anyway, I'll go ahead and merge this and just double-check that everything looks okay afterwards.

This comment has been minimized.

@njx

njx Apr 24, 2014

Member

It looks fine after merging to master - in fact, the diff history of that file doesn't even show that either of your commits affected this file (although that's probably due to the fact that git log hides some commits from history by default if they end up causing no diffs). It's a little disturbing that it ended up wrong in your branch, but all's well that ends well I guess :)

@njx

njx Apr 24, 2014

Member

It looks fine after merging to master - in fact, the diff history of that file doesn't even show that either of your commits affected this file (although that's probably due to the fact that git log hides some commits from history by default if they end up causing no diffs). It's a little disturbing that it ended up wrong in your branch, but all's well that ends well I guess :)

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Apr 24, 2014

Member

@lkcampbell Re-reviewed - just a few small notes and this is ready to go. Thanks again for doing this.

Member

njx commented Apr 24, 2014

@lkcampbell Re-reviewed - just a few small notes and this is ready to go. Thanks again for doing this.

@njx njx added the medium priority label Apr 24, 2014

@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 24, 2014

Contributor

@njx, Code Review changes committed. One question on the merge conflict for you posted above.

Contributor

lkcampbell commented Apr 24, 2014

@njx, Code Review changes committed. One question on the merge conflict for you posted above.

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Apr 24, 2014

Member

Looks good (except for the possible merge issue above - will double-check after merging). Thanks for doing this!

Member

njx commented Apr 24, 2014

Looks good (except for the possible merge issue above - will double-check after merging). Thanks for doing this!

njx added a commit that referenced this pull request Apr 24, 2014

Merge pull request #7488 from lkcampbell/fix-6084
Fix for issue #6084: Move Find items to different menu

@njx njx merged commit 4d968b2 into adobe:master Apr 24, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@lkcampbell

This comment has been minimized.

Show comment
Hide comment
@lkcampbell

lkcampbell Apr 24, 2014

Contributor

@njx, yeah, everything looks fine on my end as well. I ran the test suite and reviewed the specific code where the conflict occurred just to make sure, and everything appears to be as I expect it to be.

Contributor

lkcampbell commented Apr 24, 2014

@njx, yeah, everything looks fine on my end as well. I ran the test suite and reviewed the specific code where the conflict occurred just to make sure, and everything appears to be as I expect it to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment