Closed Bug 714942 Opened 12 years ago Closed 12 years ago

Implement minimal UI for Jump to line in the Source Editor

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 12

People

(Reporter: msucan, Assigned: msucan)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [sourceeditor])

Attachments

(1 file, 3 obsolete files)

Moving out the UI for Jump to line from the patch for bug 650345.
Attached patch proposed patch (sort of a wip) (obsolete) — Splinter Review
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!
Attachment #585549 - Flags: review?(rcampbell)
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.
Attached patch proposed patch (obsolete) — Splinter Review
Updated to use Ctrl/Cmd-J. Thanks!
Attachment #585549 - Attachment is obsolete: true
Attachment #585549 - Flags: review?(rcampbell)
Attachment #585809 - Flags: review?(rcampbell)
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.
Attachment #585809 - Flags: review-
Are the keys displayed somewhere? In a menu bar? Otherwise, how are users supposed to discover them?
(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?
(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.
(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!
(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.
(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?
(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.
Attached patch patch update 2 (obsolete) — Splinter Review
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)
Attachment #585809 - Attachment is obsolete: true
Attachment #585809 - Flags: review?(rcampbell)
Attachment #588494 - Flags: review?(rcampbell)
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.
Attachment #588494 - Flags: review?(rcampbell) → review+
(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.
A bit of cleanup, as suggested. Thanks!
Attachment #588494 - Attachment is obsolete: true
Comment on attachment 589252 [details] [diff] [review]
[in-fx-team] patch update 3

https://hg.mozilla.org/integration/fx-team/rev/b54c8094dad5
Attachment #589252 - Attachment description: patch update 3 → [in-fx-team] patch update 3
Whiteboard: [sourceeditor] → [sourceeditor][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b54c8094dad5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sourceeditor][fixed-in-fx-team] → [sourceeditor]
Target Milestone: --- → Firefox 12
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!
Keywords: dev-doc-needed
Documented:

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

Mentioned on Firefox 12 for developers.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: