Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Ignore Live Dev toggle when in a connecting state #7049

Closed
wants to merge 2 commits into from

Conversation

redmunds
Copy link
Contributor

@redmunds redmunds commented Mar 1, 2014

This is for #7014

cc @ingorichter

@ingorichter
Copy link
Contributor

Unfortunately, this is not fixing the issue. It's a great improvement and I need to click far more often too before it stops working. But then I'll see the "Unable to load Live Development page" and the "Needs to restart Chrome" Error.

@redmunds
Copy link
Contributor Author

redmunds commented Mar 4, 2014

I'm not sure exactly how we'd handle the "click on button as many times as you can and as fast as you can" case -- I think this applies to almost any button in brackets.

I think this is helpful for the case where Live Preview is connecting very slowly or connection is timing out and user is wondering what's going on (so they click again). Do you think it's worth getting this change in for Release 37, and then leave issue open to try to improve more?

@peterflynn
Copy link
Member

I've also seen some people double-click the icon right off the bat, which seems like it could cause issues too.

@ingorichter
Copy link
Contributor

@redmunds I think this is already an improvement over the previous behavior and I'm fine with merging this PR and iterate on this, if we receive more reports that users hit this issue. My behavior of constantly clicking the live preview icon was probably not even close to what a user might be doing, but it's not very unlikely too. For now, we should merge this PR and wait for feedback. @peterflynn what do you think?

@redmunds
Copy link
Contributor Author

redmunds commented Mar 5, 2014

@ingorichter I updated this branch for the new Chrome v34 API changes that I merged into master last night.

@peterflynn This change handles double-click when opening Live Preview. But closing Live Preview is much faster, so it's already INACTIVE by the time the second click arrives, so Brackets is re-opened.

Note: with Chrome v34, you will see the following error, but only once:

'CSS.getAllStyleSheets' wasn't found Object {code: -32601, message: "'CSS.getAllStyleSheets' wasn't found"} 

This is not really an error, so maybe this message should be suppressed? @jasonsanjose Is there a way to detect what version of Chrome is being used so we could suppress this if version >= 34?

@jasonsanjose
Copy link
Member

@redmunds I thought there would be some way to ask the inspector protocol what version is running, but there isn't. We'd have to modify our startup to do some user agent sniffing and make a best guess from there on whether or not to try the deleted API.

@ingorichter
Copy link
Contributor

@redmunds I'm going to give it a try later this afternoon.

@redmunds
Copy link
Contributor Author

redmunds commented Mar 7, 2014

@jasonsanjose Any way to make this message sound less like an error?

'CSS.getAllStyleSheets' wasn't found 
    Object
        code: -32601
        message: "'CSS.getAllStyleSheets' wasn't found"
        __proto__: Object

    Object
        id: 8
        method: "CSS.getAllStyleSheets"
        params: Object
        __proto__: Object

_onError

Maybe something like:

'CSS.getAllStyleSheets' wasn't found - using 'CSS.addStyleSheet/removeStyleSheet' API

@jasonsanjose
Copy link
Member

@redmunds I filed #7127 for the error logging bug.

@jasonsanjose
Copy link
Member

@ingorichter I branched off this PR into https://github.com/adobe/brackets/compare/jasonsanjose;issue-7014?expand=1. I added an extra precautions around reentrancy into open and close. Can you see if that fixes the errors causing you to see the "unable to load..." and "relaunch chrome..." dialogs?

@njx
Copy link
Contributor

njx commented Mar 10, 2014

Are we planning to merge this for Sprint 37?

@ingorichter
Copy link
Contributor

I would like to merge this. I'm currently testing it.

@jasonsanjose
Copy link
Member

I would like to yes. I'm working with @ingorichter and @redmunds to see if my branch (with Randy's fixes) fixes the additional issues Ingo was seeing.

@njx
Copy link
Contributor

njx commented Mar 10, 2014

OK, sounds good.

@njx njx added this to the Sprint 37 milestone Mar 10, 2014
@njx
Copy link
Contributor

njx commented Mar 10, 2014

Tagged sprint 37

@ingorichter
Copy link
Contributor

@jasonsanjose this looks much better now. I haven't been able to repro the dialogs. I've tested your branch with Chrome 34 Beta and Chrome 33 Stable.

@jasonsanjose
Copy link
Member

Thanks @ingorichter. I'll close this PR and open a new one for my branch, see #7148.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants