Last Comment Bug 684445 - Orion source editor should have built-in context menu
: Orion source editor should have built-in context menu
Status: RESOLVED FIXED
[sourceeditor]
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P2 normal with 2 votes (vote)
: Firefox 13
Assigned To: Mihai Sucan [:msucan]
:
Mentors:
: 715306 (view as bug list)
Depends on: 700893 726074
Blocks: 727450 710925 730061
  Show dependency treegraph
 
Reported: 2011-09-02 21:29 PDT by Jason Barnabe (np)
Modified: 2012-03-12 06:19 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (26.37 KB, patch)
2012-02-01 13:34 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review
rebased patch (26.85 KB, patch)
2012-02-09 10:55 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review
Rebase (26.39 KB, patch)
2012-02-10 07:04 PST, Michael Ratcliffe [:miker] [:mratcliffe]
no flags Details | Diff | Review
dtd fix (28.56 KB, patch)
2012-02-15 09:06 PST, Mihai Sucan [:msucan]
rcampbell: review+
Details | Diff | Review
comment fix (28.64 KB, patch)
2012-02-18 03:17 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review
added more menu items (30.54 KB, patch)
2012-02-22 13:33 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review
interdiff (5.55 KB, patch)
2012-02-22 13:35 PST, Mihai Sucan [:msucan]
rcampbell: feedback+
Details | Diff | Review
[in-fx-team] indentation fix (30.45 KB, patch)
2012-02-27 11:12 PST, Mihai Sucan [:msucan]
no flags Details | Diff | Review

Description Jason Barnabe (np) 2011-09-02 21:29:45 PDT
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 [:streetwolf] 2011-09-10 10:07:35 PDT
Problem is on Windows 7 also.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-09-19 11:14:37 PDT
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] 2011-10-07 13:26:15 PDT
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 2011-10-13 19:18:53 PDT
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] 2011-10-14 02:35:34 PDT
(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) 2011-10-27 09:11:19 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Comment 7 Mihai Sucan [:msucan] 2012-02-01 13:34:13 PST
Created attachment 593591 [details] [diff] [review]
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] 2012-02-09 10:55:19 PST
Created attachment 595816 [details] [diff] [review]
rebased patch
Comment 9 Mihai Sucan [:msucan] 2012-02-09 11:05:49 PST
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 Michael Ratcliffe [:miker] [:mratcliffe] 2012-02-10 07:04:06 PST
Created attachment 596046 [details] [diff] [review]
Rebase
Comment 11 Mihai Sucan [:msucan] 2012-02-10 07:14:07 PST
Comment on attachment 595816 [details] [diff] [review]
rebased patch

This is the correct patch for review.
Comment 12 Mihai Sucan [:msucan] 2012-02-10 07:15:09 PST
Comment on attachment 596046 [details] [diff] [review]
Rebase

This patch is missing a fix in source-editor.jsm.
Comment 13 Mihai Sucan [:msucan] 2012-02-15 09:06:39 PST
Created attachment 597430 [details] [diff] [review]
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] 2012-02-16 09:54:36 PST
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] 2012-02-17 08:27:20 PST
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) 2012-02-17 15:41:23 PST
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] 2012-02-18 03:15:15 PST
(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] 2012-02-18 03:17:39 PST
Created attachment 598513 [details] [diff] [review]
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] 2012-02-22 13:33:16 PST
Created attachment 599759 [details] [diff] [review]
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] 2012-02-22 13:35:37 PST
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.
Comment 21 Mihai Sucan [:msucan] 2012-02-23 07:53:37 PST
Green results on try:

https://tbpl.mozilla.org/?tree=Try&rev=ff8bf938434a
Comment 22 Rob Campbell [:rc] (:robcee) 2012-02-27 08:49:04 PST
Comment on attachment 599760 [details] [diff] [review]
interdiff

extra indentation in sourceeditor.dtd.

f+
Comment 23 Rob Campbell [:rc] (:robcee) 2012-02-27 08:50:16 PST
(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] 2012-02-27 11:12:14 PST
Created attachment 600981 [details] [diff] [review]
[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] 2012-02-27 11:14:22 PST
(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] 2012-02-27 23:48:20 PST
https://hg.mozilla.org/mozilla-central/rev/0c37652c28ae
Comment 27 Rob Campbell [:rc] (:robcee) 2012-03-12 06:19:32 PDT
*** Bug 715306 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.