[CLOSED] Fix for issue #6084: Move Find items to different menu

[core-ai-bot]

Issue by lkcampbell
Friday Apr 11, 2014 at 17:39 GMT
Originally opened as adobe/brackets#7488

---

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.

---

lkcampbell included the following code: https://github.com/adobe/brackets/pull/7488/commits

[core-ai-bot]

Comment by njx
Tuesday Apr 15, 2014 at 00:03 GMT

---

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 :)

[core-ai-bot]

Comment by larz0
Tuesday Apr 15, 2014 at 00:07 GMT

---

+1 for "Find"

[core-ai-bot]

Comment by JeffryBooher
Tuesday Apr 15, 2014 at 00:07 GMT

---

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.

[core-ai-bot]

Comment by njx
Tuesday Apr 15, 2014 at 00:33 GMT

---

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.

[core-ai-bot]

Comment by JeffryBooher
Tuesday Apr 15, 2014 at 00:47 GMT

---

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...

[core-ai-bot]

Comment by lkcampbell
Tuesday Apr 15, 2014 at 14:09 GMT

---

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

[core-ai-bot]

Comment by lkcampbell
Wednesday Apr 16, 2014 at 05:13 GMT

---

@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.

[core-ai-bot]

Comment by lkcampbell
Wednesday Apr 16, 2014 at 14:33 GMT

---

@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.

[core-ai-bot]

Comment by TomMalbran
Wednesday Apr 16, 2014 at 19:40 GMT

---

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.

[core-ai-bot]

Comment by lkcampbell
Wednesday Apr 16, 2014 at 21:24 GMT

---

@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: adobe/brackets#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.

[core-ai-bot]

Comment by redmunds
Wednesday Apr 16, 2014 at 22:53 GMT

---

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.

[core-ai-bot]

Comment by lkcampbell
Thursday Apr 17, 2014 at 14:48 GMT

---

@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.

[core-ai-bot]

Comment by lkcampbell
Thursday Apr 17, 2014 at 15:06 GMT

---

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

[core-ai-bot]

Comment by njx
Monday Apr 21, 2014 at 19:19 GMT

---

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

[core-ai-bot]

Comment by MarcelGerber
Monday Apr 21, 2014 at 21:21 GMT

---

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