Scratchpad should remember View options

VERIFIED FIXED in Firefox 40

Status

VERIFIED FIXED
4 years ago
2 months ago

People

(Reporter: obrufau, Assigned: beberveiga)

Tracking

39 Branch
Firefox 40
x86
Windows XP

Firefox Tracking Flags

(firefox40 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 953206
Component: Untriaged → Developer Tools: Scratchpad
(Assignee)

Comment 1

4 years ago
Created attachment 8577016 [details] [diff] [review]
1140839-scratchpad-should-remember-view-options-v1.patch

Please review my patch. Thank you very much.
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.
Attachment #8577016 - Flags: feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8582811 [details] [diff] [review]
1140839-scratchpad-should-remember-view-options-v2.patch

Tests created.
Thank you very much.
Attachment #8577016 - Attachment is obsolete: true
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.
Attachment #8582811 - Flags: review?(past)
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.
Attachment #8582811 - Flags: review?(past) → review+
Assignee: nobody → contact
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 6

3 years ago
Created attachment 8584892 [details] [diff] [review]
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. :)
Attachment #8582811 - Attachment is obsolete: true
Attachment #8584892 - Flags: review?(past)
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."
Attachment #8584892 - Flags: review?(past)
(Assignee)

Comment 8

3 years ago
Created attachment 8585076 [details] [diff] [review]
1140839-scratchpad-should-remember-view-options-v4.patch

Another attempt. :)

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

Thank you!
Attachment #8584892 - Attachment is obsolete: true
Attachment #8585076 - Flags: review?(past)
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 :)
Attachment #8585076 - Flags: review?(past) → review+

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
QA Whiteboard: [good first verify]
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

3 years ago
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][bugday-20150710]
status-firefox40: fixed → verified

Updated

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