Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix previous commit #42

Closed
wants to merge 1 commit into from
Closed

fix previous commit #42

wants to merge 1 commit into from

Conversation

ngaro
Copy link
Contributor

@ngaro ngaro commented Aug 19, 2012

  • onFailure instead of onSuccess, otherwise the html doesn't start loading
  • stop(), otherwise the html doesn't stop loading

@ngaro
Copy link
Contributor Author

ngaro commented Aug 20, 2012

Well, actually i need neither onFailure nor onSuccess, it worked just fine without the onSuccess, i didn't see any racecondition. But it never worked for me with onSuccess.

@ngaro
Copy link
Contributor Author

ngaro commented Aug 20, 2012

I'm not experienced in coupling javascript (i actually barely know any javascript) with c++, but if you think the race-condition happens because you can't be sure whether document.write or the shutting down will happen first then i don't think that's a problem. the javascript won't stop running when the c++ is shutdown first and the stop() in the javascript doesn't influence the shutting down of the c++

@cdumez
Copy link
Contributor

cdumez commented Aug 20, 2012

I have committed a fix at 368ae43 to reply to the Web UI before calling qApp->exit(). This way, this should guarantee that onSuccess() is called on Web UI side (no race condition). Hopefully, it now works for you. The remaining issue is the need for stop(). I don't know what it does and it appears to work fine without it so I haven't added back yet. Please shade from light of this.

@cdumez cdumez closed this Aug 20, 2012
@ngaro
Copy link
Contributor Author

ngaro commented Aug 20, 2012

I compiled your 368ae and noticed that:

  • the exit button looks better where you placed it
  • pressing it quits qbittorrent BUT the webpage telling that it's now
    longer running does not show.

If you compile my code and remove the stop() you will see that your browser
will still think that the page is loading after it's already visible (for
example, for firefox that loading-icon that looks like two arrows forming a
circle will keep turning )

I have committed a fix at 368ae43368ae43to reply to the Web UI before calling qApp->exit(). This way, this should
guarantee that onSuccess() is called on Web UI side (no race condition).
Hopefully, it now works for you. The remaining issue is the need for
stop(). I don't know what it does and it appears to work fine without it so
I haven't added back yet. Please shade from light of this.


Reply to this email directly or view it on GitHubhttps://github.com//pull/42#issuecomment-7871648.

@cdumez
Copy link
Contributor

cdumez commented Aug 20, 2012

The webpage was showing just fine for me on Safari. I have just tried Chrome and it does not show. I'll debug this.

@ngaro
Copy link
Contributor Author

ngaro commented Aug 20, 2012

I would use the src/webui/scripts/mocha-init.js from 0fe09aa and keep the
rest like it is now.

On Mon, Aug 20, 2012 at 7:21 PM, Christophe Dumez
notifications@github.comwrote:

The webpage was showing just fine for me on Safari. I have just tried
Chrome and it does not show. I'll debug this.


Reply to this email directly or view it on GitHubhttps://github.com//pull/42#issuecomment-7876491.

@cdumez
Copy link
Contributor

cdumez commented Aug 21, 2012

I believe I have fixed the race condition in dac0d67. It would be great if you could give it a try.
I tried Safari, Chrome and Firefox: it works consistently on all of them now.

@ngaro
Copy link
Contributor Author

ngaro commented Aug 21, 2012

works here too, let's keep it like this.

@cdumez
Copy link
Contributor

cdumez commented Aug 21, 2012

Great. Thanks for testing.

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

Successfully merging this pull request may close these issues.

2 participants