1140839 - Scratchpad should remember View options

Status: VERIFIED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[obrufau]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150307030233

Steps to reproduce:

Open Scratchpad and change the View options.
Close Scratchpad.
Open Scratchpad.


Actual results:

The view options have been reset.


Expected results:

They should have been remembered.

Comment 1

[Willian Gustavo Veiga]

Attachment: 1140839-scratchpad-should-remember-view-options-v1.patch

Please review my patch. Thank you very much.

Comment 2

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

Comment on attachment 8577016 [details] [diff] [review]
1140839-scratchpad-should-remember-view-options-v1.patch

Review of attachment 8577016 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Please add a test to make sure we don't regress this in the future.

Comment 3

[Willian Gustavo Veiga]

Attachment: 1140839-scratchpad-should-remember-view-options-v2.patch

Tests created.
Thank you very much.

Comment 4

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

Comment on attachment 8582811 [details] [diff] [review]
1140839-scratchpad-should-remember-view-options-v2.patch

Remember to flag for review once you have a patch ready. I'll probably look at this tomorrow.

Comment 5

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

Comment on attachment 8582811 [details] [diff] [review]
1140839-scratchpad-should-remember-view-options-v2.patch

Review of attachment 8582811 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice! r=me with the comments below addressed.

::: browser/app/profile/firefox.js
@@ +1479,2 @@
>  pref("devtools.scratchpad.showTrailingSpace", false);
> +pref("devtools.scratchpad.editorFontSize", 12);

Please add comments for the new preferences to the comment block above.

::: browser/devtools/scratchpad/scratchpad.js
@@ +251,5 @@
>    },
>  
>    /**
> +   * Update checkboxes states in the view menu items according
> +   * with saved preferences.

Nit, slightly better wording: "Check or uncheck view menu items according to stored preferences."

@@ +261,5 @@
> +  },
> +
> +  _updateViewMenuItem: function SP_updateViewMenuItem(preferenceName, menuId) {
> +    let checked = Services.prefs.getBoolPref(preferenceName);
> +    document.getElementById(menuId).setAttribute('checked', checked);

Don't set the attribute value to false, just remove it in that case:

https://developer.mozilla.org/docs/Mozilla/Tech/XUL/PopupGuide/MenuItems#Checkbox_menu_items

::: browser/devtools/scratchpad/test/browser.ini
@@ +41,5 @@
>  [browser_scratchpad_restore.js]
>  [browser_scratchpad_tab_switch.js]
>  [browser_scratchpad_ui.js]
>  [browser_scratchpad_close_toolbox.js]
> +[browser_bug_1140839_should_remember_view_options.js]

Nit: we don't use bug numbers in test names any more. A name like browser_scratchpad_remember_view_options.js would be better.

::: browser/devtools/scratchpad/test/browser_bug_1140839_should_remember_view_options.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function test()

Add a brief comment about the purpose of the test please.

Comment 6

[Willian Gustavo Veiga]

Attachment: 1140839-scratchpad-should-remember-view-options-v3.patch

First of all, thank you again, Panos.

I've found a comment in the wrong place, in the same block that you mentioned (browser/app/profile/firefox.js):

    // - enableCodeFolding: Whether to enable code folding or not.

I think it should be in the comment block related to the pref:

    pref("devtools.editor.enableCodeFolding", true);

How should I proceed? Should I create another patch to fix this or add it to this one?

Sorry about my weird sentences. English is not my native language. :)

Comment 7

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

Comment on attachment 8584892 [details] [diff] [review]
1140839-scratchpad-should-remember-view-options-v3.patch

Review of attachment 8584892 [details] [diff] [review]:
-----------------------------------------------------------------

About the misplaced comment, I agree with what you said. Just move the comment near the actual line that sets the pref.

::: browser/devtools/scratchpad/scratchpad.js
@@ +261,5 @@
> +
> +  _updateViewMenuItem: function SP_updateViewMenuItem(preferenceName, menuId) {
> +    let checked = Services.prefs.getBoolPref(preferenceName);
> +    if (!checked) {
> +        document.getElementById(menuId).removeAttribute('checked');

Wait, this will correctly remove the attribute when the pref is false, but it no longer sets the attribute if it's true. You want to have an if/else block here that does both.

::: browser/devtools/scratchpad/test/browser_scratchpad_remember_view_options.js
@@ +8,5 @@
> +  waitForExplicitFinish();
> +
> +  // To test for this bug we open a Scratchpad window and change all
> +  // view menu options. After each change we compare the correspondent
> +  // preference value with the expected value.

This is useful too, but what I wanted was a one line comment above "function test()" that describes the purpose of the test, e.g.: "// Test that view menu items are remembered across scratchpad invocations."

Comment 8

[Willian Gustavo Veiga]

Attachment: 1140839-scratchpad-should-remember-view-options-v4.patch

Another attempt. :)

What's the best time to find you in #devtools?

Thank you!

Comment 9

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

Comment on attachment 8585076 [details] [diff] [review]
1140839-scratchpad-should-remember-view-options-v4.patch

Review of attachment 8585076 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, thanks!

You can find me on IRC usually around normal southern Europeean times :)

Comment 10

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

https://hg.mozilla.org/integration/fx-team/rev/1e3c67376410

Comment 11

[Khalid Syfullah Zaman [:khalid32]]

Reproduced this bug by following the STR in ubuntu 14.04 LTS, 32-bit with Firefox 40.0

Build ID 	20150709163524
User Agent 	Mozilla/5.0 (X11; Linux i686; rv:40.0) Gecko/20100101 Firefox/40.0

Comment 12

[Ashickur Rahman]

Successfully reproduce the bug on Firefox 39 (20150630154324)

The fix works for me on Firefo 40; Build ID 20150709163524; User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0

Based on Comment 11 and my verification I am marking this bus as verified.