Implement a View menu in Scratchpad

RESOLVED FIXED in Firefox 31

Status

defect
RESOLVED FIXED
6 years ago
11 months ago

People

(Reporter: obrufau, Assigned: beberveiga)

Tracking

29 Branch
Firefox 31
x86
Windows XP
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 12 obsolete attachments)

36.32 KB, image/jpeg
Details
10.83 KB, patch
bbenvie
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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

5 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

5 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 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)
(Reporter)

Updated

5 years ago
Blocks: 953205
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)
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!
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 6

5 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

5 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
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

5 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.
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.
(Assignee)

Comment 13

5 years ago
Attachment #8359515 - Attachment is obsolete: true
anton, did you want to review this?

Assigning this to Willian.
Assignee: nobody → contact
Status: NEW → ASSIGNED
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!
Depends on: 964504
(Assignee)

Comment 16

5 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 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

5 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 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

5 years ago
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]
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-
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

5 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

5 years ago
Attachment #8371920 - Attachment is obsolete: true
Find someone who have a Mac. I can probably do that next week.
(Assignee)

Comment 26

5 years ago
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!
(Assignee)

Comment 30

5 years ago
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?
(Assignee)

Comment 32

5 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
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

5 years ago
I've added tests. Do I need more?
Thank you.
Attachment #8395476 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8399722 - Flags: review?(bbenvie)
(Assignee)

Comment 35

5 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 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

5 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 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+
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
https://hg.mozilla.org/integration/fx-team/rev/5cd19abab55f
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5cd19abab55f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
(Reporter)

Comment 43

5 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

5 years ago
Shouldn't View Menu be available on Aurora 31 ?
I don't believe Aurora 31 is out yet.
(Reporter)

Comment 46

5 years ago
Oh sorry, I assumed it was Aurora 31 since Nightly 32 and Stable 29 were released.
(Reporter)

Updated

4 years ago
Depends on: 1140839

Updated

3 years ago
Duplicate of this bug: 953205

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.