Closed
Bug 953206
Opened 11 years ago
Closed 11 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•11 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•11 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•11 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•11 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•11 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•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 6•11 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•11 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•11 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•11 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•11 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•11 years ago
|
||
(Except for zoom, actually)
Comment 12•11 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•11 years ago
|
||
Attachment #8359515 -
Attachment is obsolete: true
Comment 14•11 years ago
|
||
anton, did you want to review this?
Assigning this to Willian.
Assignee: nobody → contact
Status: NEW → ASSIGNED
Comment 15•11 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•11 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•11 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•11 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•11 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•11 years ago
|
||
View menu implemented. "Highlight JavaScript" feature was removed.
Attachment #8368264 -
Attachment is obsolete: true
Attachment #8371920 -
Flags: review?(anton)
Comment 21•11 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•11 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•11 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•11 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•11 years ago
|
Attachment #8371920 -
Attachment is obsolete: true
Comment 25•11 years ago
|
||
Find someone who have a Mac. I can probably do that next week.
Assignee | ||
Comment 26•11 years ago
|
||
May you test it with your Mac, benvie? Thank you!
Flags: needinfo?(bbenvie)
Comment 27•11 years ago
|
||
I'm one of those poor souls without a mac. Sorry!
Flags: needinfo?(bbenvie)
Comment 28•11 years ago
|
||
I will test it out tonight
Comment 29•11 years ago
|
||
Hi Willian, can you rebase your patch and I will test it out. Thanks!
Assignee | ||
Comment 30•11 years ago
|
||
Here it goes.
Thank you Gabriel L.
Attachment #8376734 -
Attachment is obsolete: true
Comment 31•11 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•11 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•11 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•11 years ago
|
||
I've added tests. Do I need more?
Thank you.
Attachment #8395476 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8399722 -
Flags: review?(bbenvie)
Assignee | ||
Comment 35•11 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•11 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•11 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•11 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•11 years ago
|
||
Updates the commit message to follow our format.
Attachment #8399744 -
Attachment is obsolete: true
Attachment #8399746 -
Flags: review+
Comment 41•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Reporter | ||
Comment 43•11 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•11 years ago
|
||
Shouldn't View Menu be available on Aurora 31 ?
Comment 45•11 years ago
|
||
I don't believe Aurora 31 is out yet.
Reporter | ||
Comment 46•11 years ago
|
||
Oh sorry, I assumed it was Aurora 31 since Nightly 32 and Stable 29 were released.
Updated•7 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•