Live Preview MultiBrowser improvements (#10206, #10239) #10285
Conversation
| @@ -75,6 +75,10 @@ define(function main(require, exports, module) { | |||
|
|
|||
| // current selected implementation (LiveDevelopment | LiveDevMultiBrowser) | |||
| var LiveDevImpl; | |||
|
|
|||
| // "livedev.multibrowser" preference | |||
| var PREF_MULTIBROWSER = "livedev.multibrowser"; | |||
There was a problem hiding this comment.
This actually should be PrefixedPreferencesSystem, I think.
busykai
changed the title
busykai
changed the title
|
@redmunds, this is ready for a review. |
|
@busykai I found a weird case when testing this. Let me know if I should file this:
Results I don't think it's related to this change. Let me know. |
|
@redmunds, I'm trying to reproduce this case but still couldn't do it. This is what I'm doing:
By following these steps I got LiveDev working. Please, let me know if those are representative of the steps you have described or if I misunderstood something. Thanks. |
|
@sebaslv I did not open in browser first (step 4), so not sure if that's making a difference. FYI, I'm seeing this on Win7 Firefox (latest) with WampServer 2.2 . |
|
@redmunds, Thanks for the info. I was able to reproduce it now by configuring local server, the step I missed before I think :). It seems that it never connects beacuse the page is not instrumented so the Currently we're not managing that error. I'll create a fix for that independently of this case since it's pending. The thing is that it would close the connection which is something different to what the current implementation does in this scenario. I see that it launches the page and moves the icon to a connected status but keep live editing disabled. I could figure out how to identify the case and handle that in this particular way but not sure if that is the right UX we would like for this. What do you guys suggest? |
|
@sebaslv I agree that it should be a separate issue and not hold up this PR. I'll open new issue. Seems like doing a better job of cleaning up when changing implementations should fix that. |
|
Sounds good. I'll create a PR for that issue. Thanks. |
| @@ -230,9 +230,9 @@ define(function (require, exports, module) { | |||
| }); | |||
| } | |||
|
|
|||
| function toggleErrorNotification(bool) { | |||
| function togglePref(key, bool) { | |||
| var val, | |||
There was a problem hiding this comment.
These changes to DebugCommands/main.js are not necessary. I only refactored this code from toggleErrorNotification() into togglePref() to reduce duplication with new function, but it was moved to LiveDevelopment/main.js so there's no need for any changes here.
|
@busykai Done with review. This looks good. Just need to backout changes to DebugCommands/main.js and squash commits. |
PreferencesManager.on("change") was executing before AppInit.appReady().
This was causing various inconsistencies, including setting up the UI before
LiveDevelopment is actually initialized. Handling of pref change is now moved
to AppInit.appReady and the implementation is set only after the modules are
actually initialized.
Fix #10239: toggle menu to switch livedev impls.
- Ensure correct status changes when switching implementations.
- Disable 'Project Settings' when multi-browser is on.
Also:
- Minor improvements to how the pref is defined.
- Close LiveDevelopment explicitly after each test.
- Minor refactor/doc changes in LiveDevelopment/main.js
- Minor stylistic and documentation changes.
busykai
force-pushed
the
kai/fix-10206-ld-mb-unittests-part1
branch
from
95cde63
to
eef9c68
Compare
|
@redmunds, done! |
|
Merging. |
Fix #10206 (point 1):
PreferencesManager.on("change") was executing before AppInit.appReady().
This was causing various inconsistencies, including setting up the UI before
LiveDevelopment is actually initialized. Handling of pref change is now moved
to AppInit.appReady and the implementation is set only after the modules are
actually initialized.
Fix #10239:
Toggle menu item to enable/disable multi-browser live preview. Under
File -> Enable Experimental Live Preview. Multiple fixes to implementation life-cycle to support correct state transitions on switch.Minor nits included: pref definition improved, suite name made shorter, spare lines removed, documentation improvements.
CC: @peterflynn, @sebaslv, @redmunds