Closed
Bug 953206
Opened 10 years ago
Closed 10 years ago
Implement a View menu in Scratchpad
Categories
(DevTools Graveyard :: Scratchpad, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: obrufau, Assigned: beberveiga)
References
Details
Attachments
(2 files, 12 obsolete files)
36.32 KB,
image/jpeg
|
Details | |
10.83 KB,
patch
|
bbenvie
:
review+
|
Details | Diff | Splinter Review |
I think it would be useful to have a View menu in Scratchpad. For example, it could -> Change zoom -> Change font family -> Enable/disable syntax highlighting -> Customize syntax highlighting -> Show/hide line numbers -> Enable/disable breaking long lines instead of horizontal scroll
Assignee | ||
Comment 1•10 years ago
|
||
"Change font family" and "Customize syntax highlighting" don't seem to fit well in the "View" menu item. This mockup is my proposal and both menu items weren't included.
Assignee | ||
Comment 2•10 years ago
|
||
Patch based in the mockup. It doesn't have any command. Keyboard shortcuts are missing from some items.
Attachment #8356624 -
Flags: review?(fayearthur)
Comment 3•10 years ago
|
||
Comment on attachment 8356624 [details] [diff] [review] bug-953206-menu-without-commands.patch Thanks for the patch, Willian. I'm not as familiar with the scratchpad trajectory these days and I think Rob would have more to say about this, so I'm changing review to him.
Attachment #8356624 -
Flags: review?(fayearthur) → review?(rcampbell)
Comment 4•10 years ago
|
||
Comment on attachment 8356624 [details] [diff] [review] bug-953206-menu-without-commands.patch Review of attachment 8356624 [details] [diff] [review]: ----------------------------------------------------------------- This is a good start for this menu, but we'll need the underlying commands to make these work. ::: browser/locales/en-US/chrome/browser/devtools/scratchpad.dtd @@ +52,5 @@ > > +<!ENTITY viewMenu.label "View"> > +<!ENTITY viewMenu.accesskey "V"> > + > +<!ENTITY syntaxHighlight.label "Syntax Highlight"> I would reword this to "Highlight JavaScript". @@ +53,5 @@ > +<!ENTITY viewMenu.label "View"> > +<!ENTITY viewMenu.accesskey "V"> > + > +<!ENTITY syntaxHighlight.label "Syntax Highlight"> > +<!ENTITY syntaxHighlight.accesskey "S"> maybe change this to an H. @@ +55,5 @@ > + > +<!ENTITY syntaxHighlight.label "Syntax Highlight"> > +<!ENTITY syntaxHighlight.accesskey "S"> > + > +<!ENTITY lineNumbers.label "Line Numbers"> Reword this to "Show Line Numbers". Making these menu items actions instead of things I think makes more sense. @@ +58,5 @@ > + > +<!ENTITY lineNumbers.label "Line Numbers"> > +<!ENTITY lineNumbers.accesskey "L"> > + > +<!ENTITY wordWrap.label "Word Wrap"> maybe "Wrap Text" or "Soft Wrap"?
Attachment #8356624 -
Flags: review?(rcampbell)
Comment 5•10 years ago
|
||
Re-reading the original reporter's comments, we might want to provide highlighting for different languages eventually. I'm fine with a toggle for JavaScript for a first version though. Thanks!
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 6•10 years ago
|
||
This patch is based on your review, Rob Campbell. I'll try to talk to you on IRC. Thank you very much.
Attachment #8356624 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
"Show Line Numbers" command implemented. I think we need a good keyboard shortcut for this feature. Do you have any suggestion? Thank you very much.
Attachment #8357692 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
Keyboard shortcuts is an undocumented mess. I don't think we need to bother with it in this case—I doubt many people toggle line numbers that often.
Assignee | ||
Comment 9•10 years ago
|
||
Anton, I understand what you mean and I agree with you. What do you think. Are there any menu item that needs keyboard shortcuts? Thank you.
Comment 10•10 years ago
|
||
I wouldn't bother with any shortcuts in the View menu. If it happens that there is something that could really benefit from a shortcut, we can always add it later.
Comment 11•10 years ago
|
||
(Except for zoom, actually)
Comment 12•10 years ago
|
||
Hey, I just landed bug 910183. Could you add "highlight trailing space" as an option as well, please? There's a Firefox pref devtools.scratchpad.highlightTrailingSpace and Editor pref highglightTrailingSpace that you will need to work it.
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8359515 -
Attachment is obsolete: true
Comment 14•10 years ago
|
||
anton, did you want to review this? Assigning this to Willian.
Assignee: nobody → contact
Status: NEW → ASSIGNED
Comment 15•10 years ago
|
||
Comment on attachment 8361993 [details] [diff] [review] bug-953206-menu-without-commands-v4.patch Actually, cancel that review request, Anton. There are a number of indentation changes in scratchpad.dtd. We can't include those because it cause trouble with our localization automation software. Also makes it impossible to see what's actually changed. thanks!
Assignee | ||
Comment 16•10 years ago
|
||
Text commands implemented. Should we use the "font" term or "text" ?
Attachment #8361993 -
Attachment is obsolete: true
Attachment #8366979 -
Flags: review?(anton)
Comment 17•10 years ago
|
||
Comment on attachment 8366979 [details] [diff] [review] bug-953206-menu-without-commands-v5.patch Review of attachment 8366979 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good but I'd like to see some style changes. See my inline comments. Once you're done, r? me again, I'll test and r+ it. Thanks! ::: browser/devtools/scratchpad/scratchpad.js @@ +1686,5 @@ > > + /** > + * Toggle the editor's line numbers. > + */ > + toggleLineNumbers: function SP_toggleLineNumbers() Do you really need these one-line methods? You can just do `Scratchpad.toggleEditorOption("lineNumbers")` in scratchpad.xul. @@ +1708,5 @@ > + let newOptionValue = !this.editor.getOption(optionName); > + this.editor.setOption(optionName, newOptionValue); > + }, > + > + _maximumTextSize: 96, This (and _minimumTextSize) should be file-level constants. E.g. `const MAXIMUM_TEXT_SIZE = 96;` in the beginning of the file. @@ +1712,5 @@ > + _maximumTextSize: 96, > + > + increaseTextSize: function SP_increaseTextSize() > + { > + let currentTextSize = this.editor.getFontSize(); Nit: long, descriptive variable names are good but when your function is only four lines long they don't add much. `let size = this.editor.getFontSize();` would work as well as `currentTextSize`. @@ +1730,5 @@ > + this.editor.setFontSize(currentTextSize - 1); > + } > + }, > + > + _normalTextSize: 12, This should be a constant as well. Also, how did you get that number?
Attachment #8366979 -
Flags: review?(anton) → review-
Assignee | ||
Comment 18•10 years ago
|
||
I've copied these numbers from LibreOffice Writer. Except the normal one, that I made up. Thank you very much Anton.
Attachment #8366979 -
Attachment is obsolete: true
Attachment #8368264 -
Flags: review?(anton)
Comment 19•10 years ago
|
||
Comment on attachment 8368264 [details] [diff] [review] bug-953206-menu-with-some-commands-v6.patch Review of attachment 8368264 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good. It depends on bug 964504 though which was backed out so don't land this until I re-land bug 964504. I'll do that ASAP. Thanks!
Attachment #8368264 -
Flags: review?(anton) → review+
Assignee | ||
Comment 20•10 years ago
|
||
View menu implemented. "Highlight JavaScript" feature was removed.
Attachment #8368264 -
Attachment is obsolete: true
Attachment #8371920 -
Flags: review?(anton)
Comment 21•10 years ago
|
||
Thanks, will review later today. To clarify we decided to implement "Highlight <Language>" as a followup when we actually support anything more than JavaScript in Scratchpad.
Comment 22•10 years ago
|
||
Comment on attachment 8371920 [details] [diff] [review] bug-953206-view-menu-implemented-v7.patch Review of attachment 8371920 [details] [diff] [review]: ----------------------------------------------------------------- The code looks good but there are some problems with functionality: 1) You need to add check-boxes to the View menu items when their features are enables. For example, in this screenshot (http://cl.ly/image/2V3T3n1O431r) you can see that Highlight Trailing Space feature is enabled but there's not indication of that in the View menu. 2) Alt + and Alt - shortcuts didn't work for me on a Mac. 3) When resizing text it seems like we need to redraw CodeMirror somehow, otherwise cursor becomes all weird. I'll look into it.
Attachment #8371920 -
Flags: review?(anton) → review-
Comment 23•10 years ago
|
||
Ok re 3) in your patch, could you make the setFontSize method look like this: /** * Sets font size for the editor area. */ setFontSize: function (size) { let cm = editors.get(this); cm.getWrapperElement().style.fontSize = parseInt(size, 10) + "px"; cm.refresh(); }, This will solve no. 3
Assignee | ||
Comment 24•10 years ago
|
||
Alt + and Alt shortcuts does work for me on a Debian Linux. I don't have a Mac to test it. How should we proceed now, Anton? Thank you very much.
Assignee | ||
Updated•10 years ago
|
Attachment #8371920 -
Attachment is obsolete: true
Comment 25•10 years ago
|
||
Find someone who have a Mac. I can probably do that next week.
Assignee | ||
Comment 26•10 years ago
|
||
May you test it with your Mac, benvie? Thank you!
Flags: needinfo?(bbenvie)
Comment 27•10 years ago
|
||
I'm one of those poor souls without a mac. Sorry!
Flags: needinfo?(bbenvie)
Comment 28•10 years ago
|
||
I will test it out tonight
Comment 29•10 years ago
|
||
Hi Willian, can you rebase your patch and I will test it out. Thanks!
Assignee | ||
Comment 30•10 years ago
|
||
Here it goes. Thank you Gabriel L.
Attachment #8376734 -
Attachment is obsolete: true
Comment 31•10 years ago
|
||
Hi Willian, I tested the patch on my mac and it is working great! I tested that each menu item in 'View' was working, and the shortkeys for Larger/Smaller/Normal Font Size was working as well. It looked like the shortkeys for Wrap Text and Highlight Trailing Spaces are still missing, but maybe this was intended?
Assignee | ||
Comment 32•10 years ago
|
||
Great! Yes, shortkeys for Wrap Text and Highlight Trailing Spaces are missing. This was intended. What's next? Thank you very much Gabriel L
Comment 33•10 years ago
|
||
I would look to see if there are any related unit tests for the scratchpad ui, and ensure that it covers the View menu then send a review to Benvie once you think your patch is ready.
Assignee | ||
Comment 34•10 years ago
|
||
I've added tests. Do I need more? Thank you.
Attachment #8395476 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8399722 -
Flags: review?(bbenvie)
Assignee | ||
Comment 35•10 years ago
|
||
"command" events are registered with "addEventListener".
Attachment #8399722 -
Attachment is obsolete: true
Attachment #8399722 -
Flags: review?(bbenvie)
Attachment #8399729 -
Flags: review?(bbenvie)
Comment 36•10 years ago
|
||
Comment on attachment 8399729 [details] [diff] [review] 953206-implement-a-view-menu-in-scratchpad-v11.patch Review of attachment 8399729 [details] [diff] [review]: ----------------------------------------------------------------- The test additions look good. Just one nit. ::: browser/devtools/scratchpad/scratchpad.js @@ +1809,5 @@ > + let newOptionValue = !this.editor.getOption(optionName); > + this.editor.setOption(optionName, newOptionValue); > + }, > + > + increaseTextSize: function SP_increaseTextSize() These three TextSize methods need comments describing them.
Attachment #8399729 -
Flags: review?(bbenvie) → review+
Assignee | ||
Comment 37•10 years ago
|
||
I've added some comments and "text" references were renamed to "font".
Attachment #8399729 -
Attachment is obsolete: true
Attachment #8399744 -
Flags: review?(bbenvie)
Comment 38•10 years ago
|
||
Comment on attachment 8399744 [details] [diff] [review] 953206-implement-a-view-menu-in-scratchpad-v12.patch Review of attachment 8399744 [details] [diff] [review]: ----------------------------------------------------------------- Already have r+ from me, but here's another! Thanks for addressing the feedback from here and on IRC! We'll do a try run just to be safe, then it can land. https://tbpl.mozilla.org/?tree=Try&rev=08997da4edaa
Attachment #8399744 -
Flags: review?(bbenvie) → review+
Comment 39•10 years ago
|
||
Updates the commit message to follow our format.
Attachment #8399744 -
Attachment is obsolete: true
Attachment #8399746 -
Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5cd19abab55f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Reporter | ||
Comment 43•10 years ago
|
||
Good work! But I think View menu should be moved after Edit menu, not before. Normally its placed this way.
Reporter | ||
Comment 44•10 years ago
|
||
Shouldn't View Menu be available on Aurora 31 ?
Comment 45•10 years ago
|
||
I don't believe Aurora 31 is out yet.
Reporter | ||
Comment 46•10 years ago
|
||
Oh sorry, I assumed it was Aurora 31 since Nightly 32 and Stable 29 were released.
Updated•5 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•