953206 - Implement a View menu in Scratchpad

Status: RESOLVED FIXED

Description

[obrufau]

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

Comment 1

[Willian Gustavo Veiga]

Attachment: scratchpad-view menu-proposal.jpg

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

Comment 2

[Willian Gustavo Veiga]

Attachment: bug-953206-menu-without-commands.patch

Patch based in the mockup. It doesn't have any command. Keyboard shortcuts are missing from some items.

Comment 3

[Heather Arthur [:harth]]

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.

Comment 4

[Rob Campbell [:rc] (:robcee)]

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"?

Comment 5

[Rob Campbell [:rc] (:robcee)]

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!

Comment 6

[Willian Gustavo Veiga]

Attachment: bug-953206-menu-without-commands-v2.patch

This patch is based on your review, Rob Campbell.

I'll try to talk to you on IRC.

Thank you very much.

Comment 7

[Willian Gustavo Veiga]

Attachment: bug-953206-menu-without-commands-v3.patch

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

Comment 8

[Anton Kovalyov (:anton)]

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.

Comment 9

[Willian Gustavo Veiga]

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

[Anton Kovalyov (:anton)]

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

[Anton Kovalyov (:anton)]

(Except for zoom, actually)

Comment 12

[Anton Kovalyov (:anton)]

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.

Comment 13

[Willian Gustavo Veiga]

Attachment: bug-953206-menu-without-commands-v4.patch

Comment 14

[Rob Campbell [:rc] (:robcee)]

anton, did you want to review this?

Assigning this to Willian.

Comment 15

[Rob Campbell [:rc] (:robcee)]

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!

Comment 16

[Willian Gustavo Veiga]

Attachment: bug-953206-menu-without-commands-v5.patch

Text commands implemented. Should we use the "font" term or "text" ?

Comment 17

[Anton Kovalyov (: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?

Comment 18

[Willian Gustavo Veiga]

Attachment: bug-953206-menu-with-some-commands-v6.patch

I've copied these numbers from LibreOffice Writer. Except the normal one, that I made up.

Thank you very much Anton.

Comment 19

[Anton Kovalyov (: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!

Comment 20

[Willian Gustavo Veiga]

Attachment: bug-953206-view-menu-implemented-v7.patch

View menu implemented. "Highlight JavaScript" feature was removed.

Comment 21

[Anton Kovalyov (: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 22

[Anton Kovalyov (:anton)]

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.

Comment 23

[Anton Kovalyov (:anton)]

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

Comment 24

[Willian Gustavo Veiga]

Attachment: bug-953206-view-menu-implemented-v8.patch

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.

Comment 25

[Anton Kovalyov (:anton)]

Find someone who have a Mac. I can probably do that next week.

Comment 26

[Willian Gustavo Veiga]

May you test it with your Mac, benvie? Thank you!

Comment 27

[Brandon Benvie [:benvie]]

I'm one of those poor souls without a mac. Sorry!

Comment 28

[Gabriel [:gl] (ΦωΦ)]

I will test it out tonight

Comment 29

[Gabriel [:gl] (ΦωΦ)]

Hi Willian, can you rebase your patch and I will test it out. Thanks!

Comment 30

[Willian Gustavo Veiga]

Attachment: 953206-implement-a-view-menu-in-scratchpad-v9.patch

Here it goes.
Thank you Gabriel L.

Comment 31

[Gabriel [:gl] (ΦωΦ)]

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?

Comment 32

[Willian Gustavo Veiga]

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

[Gabriel [:gl] (ΦωΦ)]

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.

Comment 34

[Willian Gustavo Veiga]

Attachment: 953206-implement-a-view-menu-in-scratchpad-v10.patch

I've added tests. Do I need more?
Thank you.

Comment 35

[Willian Gustavo Veiga]

Attachment: 953206-implement-a-view-menu-in-scratchpad-v11.patch

"command" events are registered with "addEventListener".

Comment 36

[Brandon Benvie [:benvie]]

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.

Comment 37

[Willian Gustavo Veiga]

Attachment: 953206-implement-a-view-menu-in-scratchpad-v12.patch

I've added some comments and "text" references were renamed to "font".

Comment 38

[Brandon Benvie [:benvie]]

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

Comment 39

[Brandon Benvie [:benvie]]

Attachment: 953206-implement-a-view-menu-in-scratchpad-v12.patch

Updates the commit message to follow our format.

Comment 40

[Brandon Benvie [:benvie]]

The oranges in the try run look to be unrelated.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5cd19abab55f

Comment 42

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/5cd19abab55f

Comment 43

[obrufau]

Good work!

But I think View menu should be moved after Edit menu, not before. Normally its placed this way.

Comment 44

[obrufau]

Shouldn't View Menu be available on Aurora 31 ?

Comment 45

[Panos Astithas (he/him) [:past] (please ni?)]

I don't believe Aurora 31 is out yet.

Comment 46

[obrufau]

Oh sorry, I assumed it was Aurora 31 since Nightly 32 and Stable 29 were released.