714942 - Implement minimal UI for Jump to line in the Source Editor

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

Moving out the UI for Jump to line from the patch for bug 650345.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch (sort of a wip)

Moved the code out of the patch for bug 650345.

Changed to use Ctrl-L as suggested by Paul. We also need to change the shortcut for Display in Scratchpad, because "Jump to line" takes it over. Please let me know which keyboard shortcuts we should use here and I'll update the patch accordingly. Thank you!

Comment 2

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

No. I would not change Ctrl-L to Jump To Line. It's going to break people's muscle memory and it is likely a much-less-frequently used feature than "Display". I would recommend using Ctrl-J to match the suggestion for Jump to line on Mac.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Updated to use Ctrl/Cmd-J. Thanks!

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 585809 [details] [diff] [review]
proposed patch

Please don't put keycodes in locale files. Use actual characters instead. Keys like F3 don't need to be localized at all.

Comment 5

[Dão Gottwald [:dao]]

Are the keys displayed somewhere? In a menu bar? Otherwise, how are users supposed to discover them?

Comment 6

[Mihai Sucan [:msucan]]

(In reply to Dão Gottwald [:dao] from comment #5)
> Are the keys displayed somewhere? In a menu bar? Otherwise, how are users
> supposed to discover them?

These are not displayed in a menu bar. These shortcuts are only for the "source editor" widget that other components can reuse.

We'll most-likely want an article in MDN about the editor shortcuts.

Do we not need to make these localizable?

Comment 7

[Dão Gottwald [:dao]]

(In reply to Mihai Sucan [:msucan] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > Are the keys displayed somewhere? In a menu bar? Otherwise, how are users
> > supposed to discover them?
> 
> These are not displayed in a menu bar. These shortcuts are only for the
> "source editor" widget that other components can reuse.

Doesn't this mean that there could be conflicts with other shortcuts depending on the context?

> We'll most-likely want an article in MDN about the editor shortcuts.

I think we'd want something built into Firefox.

> Do we not need to make these localizable?

Yes.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > (In reply to Dão Gottwald [:dao] from comment #5)
> > > Are the keys displayed somewhere? In a menu bar? Otherwise, how are users
> > > supposed to discover them?
> > 
> > These are not displayed in a menu bar. These shortcuts are only for the
> > "source editor" widget that other components can reuse.
> 
> Doesn't this mean that there could be conflicts with other shortcuts
> depending on the context?

No. This acts like a textarea that has its own "native" shortcuts. The user of the Source Editor component does not need to manually add shortcuts for basic features like jump to line, find/find-next/find-previous, cut/copy/paste, undo/redo and so on. These shortcuts do not work when the editor is out of focus.

> > We'll most-likely want an article in MDN about the editor shortcuts.
> 
> I think we'd want something built into Firefox.

The thought was that we want these shortcuts to work even without new UI from the "user" of the Source Editor component.

Should we just provide commands that the user of the component would have to call? So, should we delegate the responsibility for the shortcuts to the code that uses this new stuff? (the responsibility of adding keyboard shortcuts and a menuitem/button/whatever in the UI for improved discovery)

> > Do we not need to make these localizable?
> 
> Yes.

I apologize, but I am not sure if I understood the answer correctly: does it mean "yes, no need to make these shortcuts localizable"? or "yes, you need to make them localizable"?


Thank you for looking into my patches and for your feedback! Much appreciated!

Comment 9

[Dão Gottwald [:dao]]

(In reply to Mihai Sucan [:msucan] from comment #8)
> > > These are not displayed in a menu bar. These shortcuts are only for the
> > > "source editor" widget that other components can reuse.
> > 
> > Doesn't this mean that there could be conflicts with other shortcuts
> > depending on the context?
> 
> No. This acts like a textarea that has its own "native" shortcuts. The user
> of the Source Editor component does not need to manually add shortcuts for
> basic features like jump to line, find/find-next/find-previous,
> cut/copy/paste, undo/redo and so on. These shortcuts do not work when the
> editor is out of focus.

Normal textareas have nothing like find, find next or find previous. You're creating new shortcuts here. The fact that they only work when the editor is focused isn't necessarily relevant, since other shortcuts are supposed to work regardless of a text field being focused.

> > > We'll most-likely want an article in MDN about the editor shortcuts.
> > 
> > I think we'd want something built into Firefox.
> 
> The thought was that we want these shortcuts to work even without new UI
> from the "user" of the Source Editor component.
> 
> Should we just provide commands that the user of the component would have to
> call? So, should we delegate the responsibility for the shortcuts to the
> code that uses this new stuff? (the responsibility of adding keyboard
> shortcuts and a menuitem/button/whatever in the UI for improved discovery)

This sounds like a reasonable option.

> > > Do we not need to make these localizable?
> > 
> > Yes.
> 
> I apologize, but I am not sure if I understood the answer correctly: does it
> mean "yes, no need to make these shortcuts localizable"? or "yes, you need
> to make them localizable"?

I misread the question; they need to be localizable, except for keys like F3.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Mihai Sucan [:msucan] from comment #8)
> > > > These are not displayed in a menu bar. These shortcuts are only for the
> > > > "source editor" widget that other components can reuse.
> > > 
> > > Doesn't this mean that there could be conflicts with other shortcuts
> > > depending on the context?
> > 
> > No. This acts like a textarea that has its own "native" shortcuts. The user
> > of the Source Editor component does not need to manually add shortcuts for
> > basic features like jump to line, find/find-next/find-previous,
> > cut/copy/paste, undo/redo and so on. These shortcuts do not work when the
> > editor is out of focus.
> 
> Normal textareas have nothing like find, find next or find previous. You're
> creating new shortcuts here. The fact that they only work when the editor is
> focused isn't necessarily relevant, since other shortcuts are supposed to
> work regardless of a text field being focused.

Hm, the point here wasn't that we'll implement only the specific set of shortcuts in a textarea. Given it's a source editor we'd like to expand on that minimal set (find and go to line seem such basic stuff we'd like). The source editor component is more than a textarea.

(is this concept unacceptable?)


> > > > We'll most-likely want an article in MDN about the editor shortcuts.
> > > 
> > > I think we'd want something built into Firefox.
> > 
> > The thought was that we want these shortcuts to work even without new UI
> > from the "user" of the Source Editor component.
> > 
> > Should we just provide commands that the user of the component would have to
> > call? So, should we delegate the responsibility for the shortcuts to the
> > code that uses this new stuff? (the responsibility of adding keyboard
> > shortcuts and a menuitem/button/whatever in the UI for improved discovery)
> 
> This sounds like a reasonable option.

This would add more work for each devtool that uses the Source Editor.


> > > > Do we not need to make these localizable?
> > > 
> > > Yes.
> > 
> > I apologize, but I am not sure if I understood the answer correctly: does it
> > mean "yes, no need to make these shortcuts localizable"? or "yes, you need
> > to make them localizable"?
> 
> I misread the question; they need to be localizable, except for keys like F3.

Thanks for the clarification. In this case the same command has Cmd-G (on Macs) and F3 on Windows and Linux. Only Cmd-G should be localizable?

Comment 11

[Dão Gottwald [:dao]]

(In reply to Mihai Sucan [:msucan] from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > (In reply to Mihai Sucan [:msucan] from comment #8)
> > > > > These are not displayed in a menu bar. These shortcuts are only for the
> > > > > "source editor" widget that other components can reuse.
> > > > 
> > > > Doesn't this mean that there could be conflicts with other shortcuts
> > > > depending on the context?
> > > 
> > > No. This acts like a textarea that has its own "native" shortcuts. The user
> > > of the Source Editor component does not need to manually add shortcuts for
> > > basic features like jump to line, find/find-next/find-previous,
> > > cut/copy/paste, undo/redo and so on. These shortcuts do not work when the
> > > editor is out of focus.
> > 
> > Normal textareas have nothing like find, find next or find previous. You're
> > creating new shortcuts here. The fact that they only work when the editor is
> > focused isn't necessarily relevant, since other shortcuts are supposed to
> > work regardless of a text field being focused.
> 
> Hm, the point here wasn't that we'll implement only the specific set of
> shortcuts in a textarea. Given it's a source editor we'd like to expand on
> that minimal set (find and go to line seem such basic stuff we'd like). The
> source editor component is more than a textarea.

I understand that... It doesn't exactly weaken what I said.

As an extreme example, you wouldn't want Ctrl+W to stop working just because the source editor is focused. (In that sense, it is indeed just a textarea.)

> (is this concept unacceptable?)

No, it's not unacceptable, but it needs to be implemented in a way that avoids conflicting keys.

> > > The thought was that we want these shortcuts to work even without new UI
> > > from the "user" of the Source Editor component.
> > > 
> > > Should we just provide commands that the user of the component would have to
> > > call? So, should we delegate the responsibility for the shortcuts to the
> > > code that uses this new stuff? (the responsibility of adding keyboard
> > > shortcuts and a menuitem/button/whatever in the UI for improved discovery)
> > 
> > This sounds like a reasonable option.
> 
> This would add more work for each devtool that uses the Source Editor.

Sure, but if it's the only clean way...

> > I misread the question; they need to be localizable, except for keys like F3.
> 
> Thanks for the clarification. In this case the same command has Cmd-G (on
> Macs) and F3 on Windows and Linux. Only Cmd-G should be localizable?

Yes.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated the patch based on Dão's comments (thank you!).

Please let me know if further changes are needed. Thanks!

I am a bit unsure of my usage of nsIControllers - it's a new territory of XUL for me - I hope I haven't made any mistakes in how I use it. Comments welcome!

(will push the patch to the try server when it opens. most likely tomorrow)

Comment 13

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

Comment on attachment 588494 [details] [diff] [review]
patch update 2

+      "Goto Line...": [this.ui.gotoLine, this.ui],

real ellipsis here too?

do these need to be localized?

from the dtd:
 
-<!ENTITY findAgainCmd.label            "Find Again…">
+<!ENTITY findAgainCmd.label           "Find Again…">
 <!-- LOCALIZATION NOTE (findAgainCmd.key): This key is used only on Macs.
   -  Windows and Linux builds use the F3 key which is not localizable on purpose.
   -->
-<!ENTITY findAgainCmd.key              "G">
-<!ENTITY findAgainCmd.accesskey        "g">
+<!ENTITY findAgainCmd.key             "G">
+<!ENTITY findAgainCmd.accesskey       "g">

Are these left-overs from before the split?

otherwise, looks good. Might want to include this in your try run.

Comment 14

[Mihai Sucan [:msucan]]

(In reply to Rob Campbell [:rc] (robcee) from comment #13)
> Comment on attachment 588494 [details] [diff] [review]
> patch update 2
> 
> +      "Goto Line...": [this.ui.gotoLine, this.ui],
> 
> real ellipsis here too?
> 
> do these need to be localized?

See bug 650345 comment 16.


> from the dtd:
>  
> -<!ENTITY findAgainCmd.label            "Find Again…">
> +<!ENTITY findAgainCmd.label           "Find Again…">
>  <!-- LOCALIZATION NOTE (findAgainCmd.key): This key is used only on Macs.
>    -  Windows and Linux builds use the F3 key which is not localizable on
> purpose.
>    -->
> -<!ENTITY findAgainCmd.key              "G">
> -<!ENTITY findAgainCmd.accesskey        "g">
> +<!ENTITY findAgainCmd.key             "G">
> +<!ENTITY findAgainCmd.accesskey       "g">
> 
> Are these left-overs from before the split?

Will clean this up.

> otherwise, looks good. Might want to include this in your try run.

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


Thanks for the r+! Will land these patches ASAP.

Comment 15

[Mihai Sucan [:msucan]]

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

A bit of cleanup, as suggested. Thanks!

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 589252 [details] [diff] [review]
[in-fx-team] patch update 3

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

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b54c8094dad5

Comment 18

[Mihai Sucan [:msucan]]

Please document the new keyboard shortcut (Ctrl/Cmd-J), the new cmd_gotoLine command and the new UI method gotoLine(). These were added in Firefox 12. Thank you!

Comment 19

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/Tools/Using_the_Source_Editor

Mentioned on Firefox 12 for developers.