Save / restore RDM workspace when toggling

VERIFIED FIXED in Firefox 64

Status

P2
normal
VERIFIED FIXED
3 years ago
27 days ago

People

(Reporter: jryans, Assigned: gl)

Tracking

(Blocks: 3 bugs, {dev-doc-complete})

Trunk
Firefox 64
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox64 verified, firefox65 verified, firefox66 verified)

Details

(Whiteboard: [rdm-v2][dt-q])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
The existing RDM tool saves the current viewport size when you exit RDM.

We should preserve the current RDM workspace (set of viewports, etc.) and restore them when the tool opens again.

At the very least, the size should be restored to maintain parity with the existing tool.
Priority: -- → P2
Whiteboard: [multiviewport][triage] → [multiviewport]
Priority: P2 → P3
Assignee: nobody → jaideepb
(Reporter)

Comment 1

3 years ago
The way I imagine this working is roughly:

* Every so often (after X seconds or something) we would persist the state of Redux store in RDM using the asyncStorage module
* The next time you enter RDM, we retrieve that state from storage and pass it as the `initialState` to `createStore`[1]

This is kind of a quick and dirty approach because it does not re-run any side effects that might be triggered when perform each UI actions one after another.  Instead it just sets the state immediately.  A more complex variant would instead record a _log_ of all dispatched actions so that the transition of each action can be replayed correctly, but something like that is likely to be quite slow and is really meant more for debugging.

We may need to filter out certain parts of state if there are things that don't restore correctly.  I think we should at least be able to get the basics like viewport size working this way.

There are all kinds of ways to build on top of this later on: we could save different workspaces per site that you work on, for example.

For now, we just want the base of restoring settings (as many as we can do easily) when you re-enter.

[1]: http://redux.js.org/docs/api/createStore.html
Status: NEW → ASSIGNED
Flags: qe-verify?
(Reporter)

Updated

3 years ago
Flags: qe-verify? → qe-verify+
Created attachment 8767323 [details] [diff] [review]
1248619.patch [WIP]

Hello Ryan. As per your previous comment on this bug, I passed the viewports as initial state to the store. And I verified that the object is correctly applied to the store. Ideally, the RDM should load with the viewports applied, but it is not loading, and I'm trying to debug why that is happening. Could you give me some pointers on how to proceed?
Flags: needinfo?(jryans)
(Reporter)

Comment 3

3 years ago
Comment on attachment 8767323 [details] [diff] [review]
1248619.patch [WIP]

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

I've added a few comments that should get you further along, but there are more issues to look into at startup, such as device loading tries to re-run, etc.

::: devtools/client/responsive.html/index.js
@@ +153,5 @@
> +  store.subscribe(throttle(() => {
> +    const state = store.getState();
> +    const viewports = state.viewports;
> +    if (viewports && viewports.length > 0) {
> +      asyncStorage.setItem("rdm.viewports", viewports).then(() => {

As mentioned in store, you need to store the complete state.

::: devtools/client/responsive.html/store.js
@@ +21,5 @@
>      history = [];
>      shouldLog = true;
>    }
> +  const viewports = yield asyncStorage.getItem("rdm.viewports")
> +    .then(viewports => viewports);

This `then` line doesn't seem to do anything.

@@ +27,4 @@
>    let store = createStore({
>      log: shouldLog,
> +    history,
> +  })(combineReducers(reducers), {viewports});

The initial state of the store has either be "complete" (it must can all the properties full store would have) or nothing at all.  So, you can't store and set just the viewports.

It's possible we'll need to do some more tricks to initialize data outside of viewports the something reasonable at startup.  But to get started with this approach for the moment, set the full state.

Also, createStore expects the initialState to be undefined on the first run, but getItem will give back null.  You'll need to force to undefined for an falsy value.
Attachment #8767323 - Flags: feedback-
(Reporter)

Updated

3 years ago
Flags: needinfo?(jryans)
Created attachment 8771790 [details] [diff] [review]
1248619.patch

Apparently using intialState with combineReducers causes some issues due to scoping of the initialState for each reducer. Corollary is that the initialState is applied to the store when createStore but it is not properly scoped for each reducer. If my understanding is correct.
http://stackoverflow.com/questions/33749759/read-stores-initial-state-in-redux-reducer
Also, it seems that partial state can be provided in initialState. It is applied to the store and should be applied to the appropriate reducer.

In any case, I was unable to apply the initialState of the store to the viewports reducer. So I tried another approach without initialState in createStore, which passes the cachedViewport when dispatching addViewport action. It works well without the resizing glitch we saw before.
Attachment #8767323 - Attachment is obsolete: true
Attachment #8771790 - Flags: review?(jryans)
Priority: P3 → P2
QA Contact: mihai.boldan
Whiteboard: [multiviewport] → [multiviewport] [reserve-rdm]
(Reporter)

Comment 5

3 years ago
Comment on attachment 8771790 [details] [diff] [review]
1248619.patch

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

Hmm.  We can fall back to something like this if we must, but I would like to try restoring more of the RDM state than just the viewport size, so that we have a general save / restore ability, rather than special casing specific bits of data.

I don't think I follow your analysis of the StackOverflow answer you linked to.  It seems to say that you can provide data for either some or all of the reducers when using `combineReducers`, so it's actually more flexible than I imagined (I assumed you'd need specify all of them).

Did you attempt to save the entire state and restore the entire state like I mentioned?  It's not clear to me what "I was unable to apply the initialState of the store to the viewports reducer" means.
Attachment #8771790 - Flags: review?(jryans)
I did follow your instructions and attempt to save and restore the entire state (not just viewports) while creating the store. But I could not resolve the issue that the RDM viewport would not open, as it was supposed to, after the store was created with initialState.
I will look into it again, but if you understand why that is happening, do let me know.
Assignee: jaideepb → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
(Reporter)

Updated

2 years ago
No longer blocks: 1237360
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1315133
(Reporter)

Updated

2 years ago
Whiteboard: [multiviewport] [reserve-rdm] → [rdm-v2]
(Reporter)

Updated

2 years ago
Priority: P3 → P2
(Reporter)

Updated

2 years ago
Blocks: 1332053
(Reporter)

Updated

11 months ago
Blocks: 1443345
(Reporter)

Updated

7 months ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Keywords: dev-doc-needed

Updated

7 months ago
Product: Firefox → DevTools
(Reporter)

Updated

6 months ago
Assignee: jryans → nobody
Status: ASSIGNED → NEW
status-firefox47: affected → ---
(Assignee)

Updated

4 months ago
Assignee: nobody → gl
Status: NEW → ASSIGNED
(Assignee)

Comment 8

4 months ago
Created attachment 9009380 [details] [diff] [review]
Part 1: Refactor and restore the reload condition settings in RDM. [1.0]
Attachment #9009380 - Flags: review?(rcaliman)
(Assignee)

Updated

4 months ago
Attachment #9009380 - Flags: review?(rcaliman)
(Assignee)

Updated

4 months ago
Attachment #9009380 - Flags: review?(rcaliman)
(Assignee)

Updated

4 months ago
Duplicate of this bug: 1332754
(Assignee)

Comment 10

4 months ago
Created attachment 9009483 [details] [diff] [review]
Part 2: Refactor and remove the deviceType prop from viewports reducer [1.0]
Attachment #9009483 - Flags: review?(rcaliman)
(Assignee)

Updated

4 months ago
Attachment #9009483 - Attachment is obsolete: true
Attachment #9009483 - Flags: review?(rcaliman)
Comment on attachment 9009380 [details] [diff] [review]
Part 1: Refactor and restore the reload condition settings in RDM. [1.0]

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

As discussed over messaging, there's potential to replace the prop drilling for the handlers which only dispatch an action to the store (most `onToggle...` props) with a more concise, direct reference to the actions themselves, either via `mapDispatchToProps` or an alternative solution. Some ideas here: https://gist.github.com/heygrady/c6c17fc7cbdd978f93a746056f618552

But since we haven't settled on an approach yet, this code may go in as-is and we'll refactor later. Just leaving this here as a note for future reference.

::: devtools/client/responsive.html/reducers/ui.js
@@ +68,5 @@
> +      enabled : !ui.reloadOnTouchSimulation;
> +
> +    Services.prefs.setBoolPref(RELOAD_ON_TOUCH_SIMULATION, reloadOnTouchSimulation);
> +
> +    return Object.assign({}, ui, {

This sort of return statement can become a one-liner:

```
return { ...ui, reloadOnTouchSimulation };
```

Just a style preference. You're free to keep as is.
Attachment #9009380 - Flags: review?(rcaliman) → review+
Whiteboard: [rdm-v2] → [rdm-v2][dt-q]
Duplicate of this bug: 1492701
Blocks: 1493094
See Also: → bug 1491869
(Assignee)

Updated

4 months ago
Keywords: leave-open

Comment 13

4 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e089ee7f512
Part 1: Refactor and restore the reload condition settings in RDM. r=rcaliman
(Assignee)

Comment 15

3 months ago
Created attachment 9015137 [details] [diff] [review]
Part 2: Restore the previous device state in RDM. [1.0]
Attachment #9015137 - Flags: review?(rcaliman)
(Assignee)

Comment 16

3 months ago
Created attachment 9015168 [details] [diff] [review]
Part 2: Restore the previous device state in RDM. [2.0]
Attachment #9015137 - Attachment is obsolete: true
Attachment #9015137 - Flags: review?(rcaliman)
Attachment #9015168 - Flags: review?(rcaliman)
(Assignee)

Comment 17

3 months ago
Created attachment 9015171 [details] [diff] [review]
Part 2: Restore the previous device state in RDM. [3.0]
Attachment #9015168 - Attachment is obsolete: true
Attachment #9015168 - Flags: review?(rcaliman)
Attachment #9015171 - Flags: review?(rcaliman)
(Assignee)

Comment 18

3 months ago
Created attachment 9015180 [details] [diff] [review]
Part 3: Restore the previous viewport size, user agent, display pixel ratio and touch simultation properties. [1.0]
Attachment #9015180 - Flags: review?(rcaliman)
(Assignee)

Updated

3 months ago
Keywords: leave-open
Attachment #9015171 - Flags: review?(rcaliman) → review+
Comment on attachment 9015171 [details] [diff] [review]
Part 2: Restore the previous device state in RDM. [3.0]

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

::: devtools/client/responsive.html/actions/viewports.js
@@ +35,5 @@
>     * Change the viewport device.
>     */
>    changeDevice(id, device, deviceType) {
> +    return async function(dispatch) {
> +      await asyncStorage.setItem("devtools.responsive.deviceState",

Nit: I wonder whether we should guard this call to asyncStorage. It should work most of the time. But if, for any reason, the IndexedDB backing it throws an error, this will prevent changing devices in RDM. Given that asyncStorage is only used to enhance the experience by saving device state to restore, perhaps it should be allowed to fail gracefully if needed.
Attachment #9015180 - Flags: review?(rcaliman) → review+

Comment 20

3 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae15dbcedd8a
Part 2: Restore the previous device state in RDM. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2a9e0b7538
Part 3: Restore the previous viewport size, user agent, display pixel ratio and touch simultation properties. r=rcaliman

Comment 21

3 months ago
Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0c2bcc9bc6
Backed out 2 changesets for devtools/client/responsive.html/test/unit/test_change_device.js failures

Comment 22

3 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63551d28d39a
Part 2: Restore the previous device state in RDM. r=rcaliman
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1391ec87f2
Part 3: Restore the previous viewport size, user agent, display pixel ratio and touch simultation properties. r=rcaliman
I backed out this earlier for devtools/client/responsive.html/test/unit/test_change_device.js failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fb2a9e0b7538881fbe1c526d961338108ca4c967

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0c2bcc9bc616cc20d193969e0f4c14c7166600

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=204301908&repo=mozilla-inbound&lineNumber=1932

[task 2018-10-09T19:28:09.739Z] 19:28:09     INFO -  TEST-START | devtools/client/responsive.html/test/unit/test_change_device.js
[task 2018-10-09T19:28:10.200Z] 19:28:10  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/unit/test_change_device.js | xpcshell return code: 0
[task 2018-10-09T19:28:10.201Z] 19:28:10     INFO -  TEST-INFO took 457ms
[task 2018-10-09T19:28:10.202Z] 19:28:10     INFO -  >>>>>>>
[task 2018-10-09T19:28:10.204Z] 19:28:10     INFO -  PID 10489 | [10489, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2733
[task 2018-10-09T19:28:10.205Z] 19:28:10     INFO -  PID 10489 | JavaScript strict warning: resource://devtools/shared/Loader.jsm, line 222: ReferenceError: reference to undefined property "name"
[task 2018-10-09T19:28:10.207Z] 19:28:10     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-10-09T19:28:10.208Z] 19:28:10     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-10-09T19:28:10.210Z] 19:28:10     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-10-09T19:28:10.211Z] 19:28:10     INFO -  running event loop
[task 2018-10-09T19:28:10.212Z] 19:28:10     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "name"" {file: "resource://devtools/shared/Loader.jsm" line: 222}]"
[task 2018-10-09T19:28:10.214Z] 19:28:10     INFO -  devtools/client/responsive.html/test/unit/test_change_device.js | Starting
[task 2018-10-09T19:28:10.215Z] 19:28:10     INFO -  (xpcshell/head.js) | test pending (2)
[task 2018-10-09T19:28:10.217Z] 19:28:10     INFO -  PID 10489 | console.log: "[DISPATCH] action type:" "ADD_DEVICE_TYPE"
[task 2018-10-09T19:28:10.218Z] 19:28:10     INFO -  PID 10489 | console.log: "[DISPATCH] action type:" "ADD_DEVICE"
[task 2018-10-09T19:28:10.220Z] 19:28:10     INFO -  PID 10489 | console.log: "[DISPATCH] action type:" "ADD_VIEWPORT"
[task 2018-10-09T19:28:10.220Z] 19:28:10     INFO -  TEST-PASS | devtools/client/responsive.html/test/unit/test_change_device.js |  - Default device is unselected - "" == ""
[task 2018-10-09T19:28:10.222Z] 19:28:10  WARNING -  TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/unit/test_change_device.js |  - Changed to Firefox OS Flame device - "" == "Firefox OS Flame"
[task 2018-10-09T19:28:10.226Z] 19:28:10     INFO -  /builds/worker/workspace/build/tests/xpcshell/tests/devtools/client/responsive.html/test/unit/test_change_device.js:null:40
[task 2018-10-09T19:28:10.227Z] 19:28:10     INFO -  exiting test
[task 2018-10-09T19:28:10.229Z] 19:28:10     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2018-10-09T19:28:10.230Z] 19:28:10     INFO -  Unexpected exception NS_ERROR_ABORT:
[task 2018-10-09T19:28:10.231Z] 19:28:10     INFO -  _abort_failed_test@/builds/worker/workspace/build/tests/xpcshell/head.js:746:9
[task 2018-10-09T19:28:10.232Z] 19:28:10     INFO -  do_report_result@/builds/worker/workspace/build/tests/xpcshell/head.js:853:5
[task 2018-10-09T19:28:10.232Z] 19:28:10     INFO -  Assert<@/builds/worker/workspace/build/tests/xpcshell/head.js:55:5
[task 2018-10-09T19:28:10.232Z] 19:28:10     INFO -  proto.report@resource://testing-common/Assert.jsm:214:5
[task 2018-10-09T19:28:10.233Z] 19:28:10     INFO -  equal@resource://testing-common/Assert.jsm:246:3
[task 2018-10-09T19:28:10.233Z] 19:28:10     INFO -  @/builds/worker/workspace/build/tests/xpcshell/tests/devtools/client/responsive.html/test/unit/test_change_device.js:40:3
[task 2018-10-09T19:28:10.234Z] 19:28:10     INFO -  async*run_next_test/_run_next_test/<@/builds/worker/workspace/build/tests/xpcshell/head.js:1441:22
[task 2018-10-09T19:28:10.234Z] 19:28:10     INFO -  async*_run_next_test@/builds/worker/workspace/build/tests/xpcshell/head.js:1441:10
[task 2018-10-09T19:28:10.234Z] 19:28:10     INFO -  run@/builds/worker/workspace/build/tests/xpcshell/head.js:692:9
[task 2018-10-09T19:28:10.234Z] 19:28:10     INFO -  _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:219:3
[task 2018-10-09T19:28:10.235Z] 19:28:10     INFO -  _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:533:5
[task 2018-10-09T19:28:10.235Z] 19:28:10     INFO -  @-e:1:1
[task 2018-10-09T19:28:10.235Z] 19:28:10     INFO -  exiting test
Flags: needinfo?(gl)
(Assignee)

Updated

3 months ago
Flags: needinfo?(gl)

Comment 24

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/63551d28d39a
https://hg.mozilla.org/mozilla-central/rev/2d1391ec87f2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1491869

Comment 26

3 months ago
I managed to reproduce the issue using an older version of Nightly (2018-09-17) on Windows 10 x64 by using these steps: 
1. In one tab I opened the developer tools and enable RDM. 
2. I changed the width and height of the RDM view. 
3. Open another tab and opened RDM again.

I retested everything using latest Nightly on Windows 10 x64, Ubuntu 16.04 x 64 and macOS 10.13. The bug is not reproducing anymore. Even after a restart the RDM keeps the changed dimensions.

But I did notice something else. 
If in one of the tabs you change the dimensions again and then switch to the second tab, the second tab doesn't automatically change dimensions after refresh. You have to disable the RDM and enable it once again. Is that expected behaviour?
Flags: needinfo?(gl)
FF64b4 loses "rotate viewport" info on RDM re-entry

CURRENT

* enter RDM
* select some RDM preset, like "Laptop HiDPI" 900x1440 (weird portrait resolution, https://github.com/mozilla/simulated-devices/issues/29)
* rotate viewport, 1440x900
* exit RDM
* re-enter RDM
* RDM has lost "rotate viewport" result, we're back at 900x1440

EXPECTED

* RDM re-enters at 1440x900

NOTES

Still better than before, though (y)
Duplicate of this bug: 1501797
Leho, I filed bug 1503902 for this. Thank you for bringing this up!

Comment 30

2 months ago
I can see the proper behavior (at least the remembering which device was chosen) in Nightly, currently 65.0a1, but not in 64 as it says above. I have Developer Edition 64.0b6 and it does not remember which device was chosen. Does that mean this feature has moved to 65?
Irene, saving preferences incl. selected device is part of 64. Also, it works for me as expected in Firefox Developer Edition 64.0b6.

It would be great if you can test again and, potentially, post STR if it doesn't work for you.

Comment 32

2 months ago
> Irene, saving preferences incl. selected device is part of 64. Also, it
> works for me as expected in Firefox Developer Edition 64.0b6.
> 
> It would be great if you can test again and, potentially, post STR if it
> doesn't work for you.

I don't know why this was not working yesterday. I have not even reset my system since then. But I can no longer reproduce the problem. Can I confirm which settings should be saved between sessions?

Is it only device right now or are things like the throttling supposed to be saved as well? From what I am seeing, only the device choice is saved right now, not the connection throttling or the orientation. Is there something else that is not visible that I should be mentioning?

And thanks for your help on this.(In reply to Martin Balfanz [:mbalfanz] from comment #31)

Updated

2 months ago
Keywords: dev-doc-needed → dev-doc-complete

Comment 33

2 months ago
Added this note to the Responsive Design page:

Note: The device you select when in Responsive Design Mode will be saved between sessions.

I know other work is planned, so this will most likely get updated in the near future.

This is the release notes paragraph:

Responsive Design Mode device selection is now saved between sessions (bug 1248619).
(Assignee)

Comment 34

2 months ago
(In reply to Oana Botisan from comment #26)
> I managed to reproduce the issue using an older version of Nightly
> (2018-09-17) on Windows 10 x64 by using these steps: 
> 1. In one tab I opened the developer tools and enable RDM. 
> 2. I changed the width and height of the RDM view. 
> 3. Open another tab and opened RDM again.
> 
> I retested everything using latest Nightly on Windows 10 x64, Ubuntu 16.04 x
> 64 and macOS 10.13. The bug is not reproducing anymore. Even after a restart
> the RDM keeps the changed dimensions.
> 
> But I did notice something else. 
> If in one of the tabs you change the dimensions again and then switch to the
> second tab, the second tab doesn't automatically change dimensions after
> refresh. You have to disable the RDM and enable it once again. Is that
> expected behaviour?

Yes. The current behavior currently does not account for different tabs.
Flags: needinfo?(gl)

Updated

a month ago
Duplicate of this bug: 1513389

Updated

a month ago
See Also: bug 1491869

Comment 36

27 days ago
I can Confirm this issue as Fixed in Firefox 64.0, Nightly 66.0a1 (2018-12-19) and Beta 65.0b5.
Status: RESOLVED → VERIFIED
status-firefox64: fixed → verified
status-firefox65: --- → verified
status-firefox66: --- → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.