650345 - Implement minimal UI for Find in the Source Editor

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

We need to allow developers to do search and replace operations in the source code, within Workspace.

Comment 2

[Mihai Sucan [:msucan]]

This feature needs to be supported in the Source Editor.

Comment 3

[Mihai Sucan [:msucan]]

Going to morph this bug into:

- add a minimal UI for find/find next/find previous (with a prompt() as agreed on IRC)
- add a minimal UI for jump to line (we already have the API in bug 687160).

Both are trivial to do and they'd be really nice usability wins for Firefox 11.

Will open a follow up bug to improve the UI for these two features. I will also open a follow up to get find *and* replace support in the Source Editor component.

This will fix bug 710925.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: proposed patch (wip)

Proposed patch, no tests yet. Functionality should be complete.

Notes on the code:

- I added a JSM to keep UI stuff away from the main "text editor widget" code. I expect the UI stuff will grow to be much bigger when we add more and more features. Also, SourceEditorUI is made reusable with any SourceEditor component.

- I did put some generic code in the main source-editor.jsm that implements features that use the public Source Editor APIs. This is code that is reusable with any component. This is the case with find/findNext/findPrevious, etc.

Please let me know if you have comments on how things stand. Thank you!

(I will add tests ASAP.)

Comment 5

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Updated patch, with fixes and a test for the new functionality.

I changed the keyboard shortcuts to avoid collisions with existing shortcuts.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: only the changes since yesterday

To make reviewing easier, here are the changes I did since yesterday: fixes and the new test.

Comment 7

[Mihai Sucan [:msucan]]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=72d249e4d0b8

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 582551 [details] [diff] [review]
only the changes since yesterday

I don't think we can hard-code these values.

I also think they need to be different on each platform.

DOM_VK_F is probably fine for "Find" on most platforms. DOM_VK_F3 makes sense on Windows, though not on mac which uses DOM_VK_G for find next.

Why DOM_VK_G for go to line? I would have thought L would be a better choice? (I guess "Go to line").

Unfortunately, I don't think we can do this without making the keys localizeable. Either a properties file or figure out how to wire up commands and keys in xul.

Comment 9

[Paul Rouget [:paul]]

(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> Why DOM_VK_G for go to line? I would have thought L would be a better
> choice? (I guess "Go to line").

In view-source we use ctrl-l.

Comment 10

[Rob Campbell [:rc] (:robcee)]

looking at this again, I don't think we can use F3 on Mac. It needs to be Cmd-G for Find Next, which means we need an alternate key for Goto Line.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch. Made the keys localizable. I also moved out the code for the Jump to line UI, see bug 714942, as requested.

I'd like a decision on which keyboard shortcuts to use for find, find next and find previous. If needed, we can make different shortcuts for each OS. Please let me know. Thanks!

Also, I think this is the place where I should make the change for bug 714832. Shall I do it here in this patch? I already touch the relevant lines of code.

Looking forward for your review. Thank you!


(apologies for the time it took to update the patch - holidays took over!)

Comment 12

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Updated to use the keyboard shortcuts we decided on IRC:

https://etherpad.mozilla.org/x0WpxA9bcm

Thank you!

Comment 13

[Mihai Sucan [:msucan]]

Attachment: patch update 4

Updated based on Dão's comments in bug 714942.

This UI is now only available in Scratchpad. For the Style Editor and for the JS debugger we should open separate bugs. For example, the Style Editor needs a menu (afaik there's a bug on that, can't remember the number now).

Please let me know if further changes are needed. Thank you!

Comment 14

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588492 [details] [diff] [review]
patch update 4

in scratchpad.xul

+  <key id="key_findPrevious"
+       key="&findPreviousCmd.key;"
+       command="cmd_findPrevious"
+       modifiers="accel"/>

should be modifiers="accel,shift"

This looks very nice. R+ with a successful run through try and the above change.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 588492 [details] [diff] [review]
patch update 4

in source-editor-orion.jsm

+      "Find...": [this.ui.find, this.ui],

do these show up anywhere visible?

if so, you should use a real ellipsis instead of the '...'. i.e., '…'.

Comment 16

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #14)
> Comment on attachment 588492 [details] [diff] [review]
> patch update 4
> 
> in scratchpad.xul
> 
> +  <key id="key_findPrevious"
> +       key="&findPreviousCmd.key;"
> +       command="cmd_findPrevious"
> +       modifiers="accel"/>
> 
> should be modifiers="accel,shift"

True. I forgot this when I wrote this part of the code. I actually had test failures on Macs because of this small issue. Fixed.


> This looks very nice. R+ with a successful run through try and the above
> change.

Thanks!

I did push the fixed patch to the try server. Green results:

https://tbpl.mozilla.org/?tree=Try&rev=b711b368e4b1


(In reply to Rob Campbell [:rc] (robcee) from comment #15)
> Comment on attachment 588492 [details] [diff] [review]
> patch update 4
> 
> in source-editor-orion.jsm
> 
> +      "Find...": [this.ui.find, this.ui],
> 
> do these show up anywhere visible?

No. These are names for Orion actions, they are part of the Orion API stuff. The naming is chosen to match common action names from upstream. Thanks for checking on this - it's a valid concern, but luckily these names are never displayed.

Comment 17

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] patch update 5

Fixed the keyboard shortcut.

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 589249 [details] [diff] [review]
[in-fx-team] patch update 5

https://hg.mozilla.org/integration/fx-team/rev/5104705ee96a

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5104705ee96a

Comment 20

[Mihai Sucan [:msucan]]

Please document the addition of the following in Firefox 12:

- new source-editor-overlay.xul that needs to be included in the XUL where SourceEditor is to be used.
  - <commandset id="sourceEditorCommands"/> also needs to be added into the XUL document, to make the SourceEditor commands available.

- new SourceEditorUI JSM that provides a common UI to any Source Editor component.
  - this has its own nsIController (the SourceEditorController) that implements common editor commands. For now it has cmd_find, cmd_findAgain and cmd_findPrevious.
  - the SourceEditorUI is not generally meant to be used by any other extension or code, except those extensions that write new SourceEditor components. To use the UI methods one must have a good reason.
    - the UI has methods to invoke the find/find next/find previous actions.
    - to invoke the commands use the global commands, for example do goDoCommand("cmd_find"). This call is handled by the SourceEditorController which passes the request to the SourceEditorUI that in turn uses the SourceEditor component to perform the find.

- new methods for the SourceEditor instances: find(), findNext() and findPrevious().

- new properties for the SourceEditor instances: ui and lastFind. See source-editor.jsm.

- new keyboard shortcuts: Ctrl-F (Find), F3 (Find next on Windows/Linux), Shift-F3 (Find previous on Windows/Linux), Ctrl-G (Find next on Macs), Ctrl-Shift-G (Find previous on Macs).

- and anything I missed to mention now. ;)

For usage examples please see the new tests and the changes in scratchpad.js and scratchpad.xul.

Thank you!