684445 - Orion source editor should have built-in context menu

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Jason Barnabe (np)]

I'm using the Orion source editor in my extension Stylish. I think it would be useful if Orion came with a built-in context menu for actions like undo, redo, copy, paste, etc. This context menu could be overlayed to include window-specific items or even replaced with a whole other menu. This would eliminate the need for me (or anyone else) to implement my own context code.

Comment 1

[Gary [:streetwolf52]]

Problem is on Windows 7 also.

Comment 2

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

You can create a context menu for the editor, but agree that it would be nice to provide a default.

see, http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#718

for an example of adding a context menu handler.

Comment 3

[Mihai Sucan [:msucan]]

This is something we can solve in Source Editor code, to allow users of the JSM to provide a custom context menu, both for the Orion and textarea components. (not just for orion)

Comment 4

[Bruce A. Wittmeier]

I have some related comments.

Using scratchpad thru Tools/Web Development/Scratchpad when pasting code into the window the code does not get line numbers assigned.

However if you paste the same code to a text editor such as notepad then copy from notepad and paste to scratchpad, it works fine.

Before line numbers are assigned you cannot touch the code - it gets deleted from scratchpad.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to Bruce A. Wittmeier from comment #4)
> I have some related comments.
> 
> Using scratchpad thru Tools/Web Development/Scratchpad when pasting code
> into the window the code does not get line numbers assigned.
> 
> However if you paste the same code to a text editor such as notepad then
> copy from notepad and paste to scratchpad, it works fine.
> 
> Before line numbers are assigned you cannot touch the code - it gets deleted
> from scratchpad.

This is a known separate bug, see bug 684862. Should be fixed in the latest Firefox nightlies.

Comment 6

[Dave Camp (:dcamp)]

We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

Comment 7

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. This adds a default context menu to the Source Editor and it makes the Style Editor changes needed to be able to benefit from the new feature. Also updated Scratchpad accordingly, to use the new context menu config from the Source Editor.

Looking forward to your review! Thanks!

Comment 8

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Comment 9

[Mihai Sucan [:msucan]]

The new SourceEditor initialization option, contextMenu, needs to be documented, please see the patch. Also the new overlay features: the #sourceEditorContextMenu menupopup and the two new se-cmd-undo and se-cmd-redo commands. Usage examples are in the styleeditor.xul, scratchpad.js and scratchpad.xul. Thank you!

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebase

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 595816 [details] [diff] [review]
rebased patch

This is the correct patch for review.

Comment 12

[Mihai Sucan [:msucan]]

Comment on attachment 596046 [details] [diff] [review]
Rebase

This patch is missing a fix in source-editor.jsm.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: dtd fix

Added the new sourceeditor.dtd to the browser/locales/jar.mn file. Somehow this chunk of code got lost in the patch rebases and Firefox didn't throw any errors. Big thanks to Mike for his help with reporting and identifying the problem!

Comment 14

[Mihai Sucan [:msucan]]

Do we want to add Find, find again and jump to line in the default context menu? The current context menu I implemented here has the same options as the default textarea context menu. I would say it makes sense for a source editor to have more options, but that argument is also a slippery slope to add too many items - I'd like us to be picky on what we add.

Comment 15

[Mihai Sucan [:msucan]]

Try server result are very much green:

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

(pushed this patch and all the dependencies)

Comment 16

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

Comment on attachment 597430 [details] [diff] [review]
dtd fix

in source-editor-overlay.xul:

+
+  <!-- This Source Editor overlay requires that the editMenuOverlay.xul is
+       loaded, and you also need the globalOverlay.js script in the XUL document
+       where the source-editor-overlay.xul is loaded. -->

This comment is a little confusing. I think some slight rewording makes it clearer:

+  <!-- This Source Editor overlay requires the editMenuOverlay.xul to
+       be loaded. The globalOverlay.js script is also required in the
+       XUL document where the source-editor-overlay.xul is loaded. -->

+  _onUndoRedo: function SEU__onUndoRedo()
+  {
+    if (this._ownerWindow.goUpdateCommand) {
+      this._ownerWindow.goUpdateCommand("se-cmd-undo");
+      this._ownerWindow.goUpdateCommand("se-cmd-redo");
+    }

not really sure this needs to be in a separate method.

could just put that into _onDirtyChanged directly.

no tests with this. I believe we talked about this being somewhat difficult to write a test that actually works properly due to widget issues.

Comment 17

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #16)
> Comment on attachment 597430 [details] [diff] [review]
> dtd fix
> 
> in source-editor-overlay.xul:
> 
> +
> +  <!-- This Source Editor overlay requires that the editMenuOverlay.xul is
> +       loaded, and you also need the globalOverlay.js script in the XUL
> document
> +       where the source-editor-overlay.xul is loaded. -->
> 
> This comment is a little confusing. I think some slight rewording makes it
> clearer:
> 
> +  <!-- This Source Editor overlay requires the editMenuOverlay.xul to
> +       be loaded. The globalOverlay.js script is also required in the
> +       XUL document where the source-editor-overlay.xul is loaded. -->

Thanks! My English!... :)


> +  _onUndoRedo: function SEU__onUndoRedo()
> +  {
> +    if (this._ownerWindow.goUpdateCommand) {
> +      this._ownerWindow.goUpdateCommand("se-cmd-undo");
> +      this._ownerWindow.goUpdateCommand("se-cmd-redo");
> +    }
> 
> not really sure this needs to be in a separate method.
> 
> could just put that into _onDirtyChanged directly.

I thought I can do that, but there are still cases where undo/redo need to be tracked, and the dirty change state event is not enough.

Comment 18

[Mihai Sucan [:msucan]]

Attachment: comment fix

Thanks Rob for the r+!

The last patch needing a review in this patch queue is the one in bug 700893.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: added more menu items

Updated the patch to include find, find again and jump to line in the default context menu.

Comment 20

[Mihai Sucan [:msucan]]

Attachment: interdiff

Interdiff to make it easier for you to glance over the latest changes. Take a quick look and let me know if it's fine.

One note: it would be nice if we'd have less keys defined in scratchpad.xul and more editMenuKeys reused - like I did in the style editor code. I could remove more keys from scratchpad and replace them with the editMenuKeys.

Comment 21

[Mihai Sucan [:msucan]]

Green results on try:

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

Comment 22

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

Comment on attachment 599760 [details] [diff] [review]
interdiff

extra indentation in sourceeditor.dtd.

f+

Comment 23

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

(In reply to Mihai Sucan [:msucan] from comment #20)
> Created attachment 599760 [details] [diff] [review]
> interdiff
> 
> Interdiff to make it easier for you to glance over the latest changes. Take
> a quick look and let me know if it's fine.
> 
> One note: it would be nice if we'd have less keys defined in scratchpad.xul
> and more editMenuKeys reused - like I did in the style editor code. I could
> remove more keys from scratchpad and replace them with the editMenuKeys.

yeah, agreed. We should file a follow-up bug to do that.

Comment 24

[Mihai Sucan [:msucan]]

Attachment: [in-fx-team] indentation fix

Thanks Rob for the f+!

Landed this patch in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/0c37652c28ae

Comment 25

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #23)
> (In reply to Mihai Sucan [:msucan] from comment #20)
> > One note: it would be nice if we'd have less keys defined in scratchpad.xul
> > and more editMenuKeys reused - like I did in the style editor code. I could
> > remove more keys from scratchpad and replace them with the editMenuKeys.
> 
> yeah, agreed. We should file a follow-up bug to do that.

Filed bug 730898.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/0c37652c28ae