1275520 - Devices removed from the list reappear

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[Benoit Chabod [:bigben]]

Bug 1241714 added a device selector, with presets for the RDM.
This list is editable, meaning you can choose to add devices known by Firefox but not featured by default, or you can choose to remove some.
Added devices are correctly stored in preferences and if you refresh the RDM, they will stay there. However, removed devices keep reappearing.

[Steps to reproduce]:
1. Launch Firefox.
2. From about:config, enable the devtools.responsive.html.enabled pref.
3. Open RDM.
4. Uncheck a few devices featured by default.
5. Verify that they have been removed from the list.
6. Close the current RDM view and open another one.

[Expected result]:
- Removed devices are not visible.

[Actual result]:
- Removed devices have been added to the list again.

Comment 1

[Benoit Chabod [:bigben]]

I'm willing to fix this bug, if

Comment 2

[Benoit Chabod [:bigben]]

*I'm willing to fix this bug, if you guys agree.
This is due to a design flaw in the device module for RDM [1].
Currently, a device is displayed when this condition is fulfilled:

deviceList.has(device.name) ? true : !!device.featured

Thus, when refreshing the RDM view, if a device has been unchecked but it was a featured device, it will be re-added automatically. I guess we could fix this by changing the deviceList data structure so that it also contains devices that have been explicitly removed by the user.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsive.html/devices.js

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for noticing this.  The behavior of the `featured` flag has been particularly tricky to get correct.  I think no matter what change we actually make, we should take this opportunity to add more test coverage of the devices module, especially with changes to the input set (as if the CDN data set changed over time).

Part of the problem is I don't think we ever wrote down a very detailed description of its behavior, so let me make an attempt.

1. We have a device list with:

* Foo (featured = false, modifiedByUserTo = no change)
* Bar (featured = false, modifiedByUserTo = no change)
* Bob (featured = true, modifiedByUserTo = no change)

The selector now shows: Bob

2. User uses modal to enable device Bar and disable Bob:

* Foo (featured = false, modifiedByUserTo = no change)
* Bar (featured = false, modifiedByUserTo = enabled)
* Bob (featured = true, modifiedByUserTo = disabled)

The selector now shows: Bar

3. We update the device list to change featured flags:

* Foo (featured = true, modifiedByUserTo = no change)
* Bar (featured = false, modifiedByUserTo = enabled)
* Bob (featured = true, modifiedByUserTo = disabled)

The selection now shows: Foo, Bar

In summary, the "featured" devices make up the base set that is displayed when we don't have any other user decision to disable / enable.  If the user changes away from the featured setting to enable / disable, that is recorded and saved.

A device is displayed in the list if: `(featured && !(modifiedByUserTo == disabled)) || (modifiedByUserTo == enabled)`.  We could either use a tri-state value to store the user modification or two separate sets for "enabledByUser" and "disabledByUser".

Does all of this sound like it would act they way you would expect as a user?  Am I making this too complicated? :)

Comment 4

[Benoit Chabod [:bigben]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Thanks for noticing this.  The behavior of the `featured` flag has been
> particularly tricky to get correct.  I think no matter what change we
> actually make, we should take this opportunity to add more test coverage of
> the devices module, especially with changes to the input set (as if the CDN
> data set changed over time).

I think you're right, the device list module should be tested more thoroughly.
A few use cases should be enough to make sure this kind of bug never appears again.

> A device is displayed in the list if: `(featured && !(modifiedByUserTo ==
> disabled)) || (modifiedByUserTo == enabled)`.  We could either use a
> tri-state value to store the user modification or two separate sets for
> "enabledByUser" and "disabledByUser".
> 
> Does all of this sound like it would act they way you would expect as a
> user?  Am I making this too complicated? :)

No, this sounds good to me!
What I like about your summary is that it contains every interesting case in 3 steps: featured but disabled, not featured but enabled, and newly featured. I will try to implement this.

Comment 5

[Benoit Chabod [:bigben]]

Attachment: 1st patch - improve RDM displayed device list preference

Here's my first attempt at a patch to fix this bug!

I've opted for a new data structure containing two sets, "added" (enabled by user) and "removed" (disabled by user).
The logic used to deduce the displayed flag from the featured flag has been changed accordingly, as you suggested:

displayed: (deviceList.added.has(device.name) ||  device.featured && !(deviceList.removed.has(device.name)))

The update logic has changed as well.
I chose to store in "added" and "removed" only devices explictly added or removed, so by default the preference is now empty.

I also used this opportunity to add many tests in browser_device_modal_submit.js: add a device not featured, remove a featured device, refresh the RDM and check what happens, simulate the arrival of a new featured device, etc.

Ryan, I know you're quite busy so please don't hesitate to pass the review flag over if needed :)

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8762088 [details] [diff] [review]
1st patch - improve RDM displayed device list preference

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

Overall this looks like the right approach to me.

Just a few things to clean up before it's ready to go.

Sorry for the long wait!  Should be faster now that we're back from the work week.

::: devtools/client/responsive.html/components/device-modal.js
@@ +52,5 @@
>        onUpdateDeviceDisplayed,
>        onUpdateDeviceModalOpen,
>      } = this.props;
>  
> +    let displayedDevicePref = {

Maybe `preferredDevices`?

::: devtools/client/responsive.html/devices.js
@@ +29,5 @@
>        }
>  
>        let newDevice = Object.assign({}, device, {
> +        displayed: (deviceList.added.has(device.name) ||
> +          device.featured && !(deviceList.removed.has(device.name))),

Wrap the subexpression `device.featured && !(deviceList.removed.has(device.name)` in parens to emphasize that they go together.  Does the entire value of displayed need outer parens like it is now? Remove if possible.

@@ +34,3 @@
>        });
>  
> +      // If the featured flag happens to match the user preference, clear it

The added and removed sets mark the devices the user has expressed specific choices about.  I don't think it's necessary to "normalize" them like this with the value of featured because we want the sets to remain about the user's intent.

@@ +50,2 @@
>   *
> + * @return {Object} containing two Set:

Nit: two Sets

@@ +54,2 @@
>   */
>  function loadDeviceList() {

It's not really just a list anymore, so maybe `loadPreferredDevices` or something?  The update method's name should match too.

@@ +54,3 @@
>   */
>  function loadDeviceList() {
> +  let deviceList = {

List is not correct for these variable names, so maybe just `devices` and `saved`, but maybe there is a better name.

@@ +67,5 @@
> +      if (savedList.added && savedList.removed) {
> +        deviceList.added = new Set(savedList.added);
> +        deviceList.removed = new Set(savedList.removed);
> +      } else if (Array.isArray(savedList)) {
> +        deviceList.added = new Set(savedList);

I don't think this migration is correct, because the previous array included both user added _and_ featured devices.  But we only want user added to be in added.

Since this feature hasn't been shipped to end users, I'd say let's forget about migration.  Just check for added and removed, and do nothing if they are missing.

@@ +80,5 @@
>  
>  /**
>   * Update the displayed device list preference with the given device list.
>   *
> + * @param  {Object} containing two Set:

Nit: two Sets, only one space before {

@@ +87,3 @@
>   */
>  function updateDeviceList(devices) {
> +  devices.added = Array.from(devices.added);

It's strange to mutate the input data like this, so do something else, such as:

let devicesToSave = {
  added: Array.from(devices.added),
  ...
};

::: devtools/client/responsive.html/test/browser/browser_device_modal_exit.js
@@ +29,5 @@
>    ok(modal.classList.contains("hidden"),
>      "The device modal is hidden on exit.");
>  
>    info("Check that the device list remains unchanged after exitting.");
>    let deviceListAfter = loadDeviceList();

The word "list" in the variable names is similarly confusing here like in the other cases.

::: devtools/client/responsive.html/test/browser/browser_device_modal_submit.js
@@ +3,5 @@
>  
>  "use strict";
>  
>  // Test submitting display device changes on the device modal
> +const { GetDevices } = require("devtools/client/shared/devices");

Should be able to do { GetDevices, AddDevice }

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Oh, I forgot to add: thanks for the additional tests, it should help a lot!  This is a particularly tricky thing to get right.

Comment 8

[Benoit Chabod [:bigben]]

Attachment: 2nd patch - improve RDM displayed device list preference

No problem for the delay!
I followed your advices, here's a new version of the patch.

Most changes with the previous version concern methods/vars names.
I was planning to do it in Bug 1275559, but we'll see which one lands first :p
I've removed the migration step for the user preferences, and the normalizing thing as well.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8764319 [details] [diff] [review]
2nd patch - improve RDM displayed device list preference

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

Great, thanks for working on this!

Assuming try looks good, it seems ready to go.

Comment 10

[Benoit Chabod [:bigben]]

Thanks for the review!
Here's a try push, it seems failures are non-related intermittents only:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa9f14b5f5fc

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4323c9454ec3
Improve RDM displayed device list preference. r=jryans

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4323c9454ec3

Comment 13

[Camelia Badau [:cbadau], Desktop QA]

I've reproduced the initial issue on Nightly 2016-05-25.

Verified fixed on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OSX 10.9.5 using latest Nightly 50.0a1 (buildID: 20160629030209): the removed devices are not visible.