Closed Bug 953206 Opened 11 years ago Closed 10 years ago

Implement a View menu in Scratchpad


(DevTools Graveyard :: Scratchpad, defect)

29 Branch
Windows XP
Not set


(Not tracked)

Firefox 31


(Reporter: obrufau, Assigned: beberveiga)




(2 files, 12 obsolete files)

36.32 KB, image/jpeg
10.83 KB, patch
: 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
"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.
Patch based in the mockup. It doesn't have any command. Keyboard shortcuts are missing from some items.
Attachment #8356624 - Flags: review?(fayearthur)
Comment on attachment 8356624 [details] [diff] [review]

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)
Blocks: 953205
Comment on attachment 8356624 [details] [diff] [review]

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)
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.

Ever confirmed: true
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
"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
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.
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.
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.
(Except for zoom, actually)
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.
Attachment #8359515 - Attachment is obsolete: true
anton, did you want to review this?

Assigning this to Willian.
Assignee: nobody → contact
Comment on attachment 8361993 [details] [diff] [review]

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.

Depends on: 964504
Text commands implemented. Should we use the "font" term or "text" ?
Attachment #8361993 - Attachment is obsolete: true
Attachment #8366979 - Flags: review?(anton)
Comment on attachment 8366979 [details] [diff] [review]

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-
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 on attachment 8368264 [details] [diff] [review]

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+
View menu implemented. "Highlight JavaScript" feature was removed.
Attachment #8368264 - Attachment is obsolete: true
Attachment #8371920 - Flags: review?(anton)
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 on attachment 8371920 [details] [diff] [review]

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 ( 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-
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";

This will solve no. 3
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.
Attachment #8371920 - Attachment is obsolete: true
Find someone who have a Mac. I can probably do that next week.
May you test it with your Mac, benvie? Thank you!
Flags: needinfo?(bbenvie)
I'm one of those poor souls without a mac. Sorry!
Flags: needinfo?(bbenvie)
I will test it out tonight
Hi Willian, can you rebase your patch and I will test it out. Thanks!
Here it goes.
Thank you Gabriel L.
Attachment #8376734 - Attachment is obsolete: true
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?
Yes, shortkeys for Wrap Text and Highlight Trailing Spaces are missing. This was intended.
What's next?
Thank you very much Gabriel L
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.
I've added tests. Do I need more?
Thank you.
Attachment #8395476 - Attachment is obsolete: true
Attachment #8399722 - Flags: review?(bbenvie)
"command" events are registered with "addEventListener".
Attachment #8399722 - Attachment is obsolete: true
Attachment #8399722 - Flags: review?(bbenvie)
Attachment #8399729 - Flags: review?(bbenvie)
Comment on attachment 8399729 [details] [diff] [review]

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+
I've added some comments and "text" references were renamed to "font".
Attachment #8399729 - Attachment is obsolete: true
Attachment #8399744 - Flags: review?(bbenvie)
Comment on attachment 8399744 [details] [diff] [review]

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.
Attachment #8399744 - Flags: review?(bbenvie) → review+
Updates the commit message to follow our format.
Attachment #8399744 - Attachment is obsolete: true
Attachment #8399746 - Flags: review+
The oranges in the try run look to be unrelated.
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Good work!

But I think View menu should be moved after Edit menu, not before. Normally its placed this way.
Shouldn't View Menu be available on Aurora 31 ?
I don't believe Aurora 31 is out yet.
Oh sorry, I assumed it was Aurora 31 since Nightly 32 and Stable 29 were released.
Depends on: 1140839
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.