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

Reload tabs when the computer resumes #495

Closed
wants to merge 5 commits into from

Conversation

yuya-oc
Copy link
Contributor

@yuya-oc yuya-oc commented Mar 27, 2017

Before submitting, please confirm you've

Please provide the following information:

Summary
Reload tabs when the computer resumes.

If the computer is offline when resuming from sleep, the app doesn't reload any tabs to keep their contents.

Issue link
#461

Test Cases
Tested on macOS 10.12.3

Case 1:

  1. Let the computer sleep.
  2. Resume from sleep.
  3. The app should reload all tabs immediately.

Case 2:

  1. Turn off Wi-Fi or unplug LAN cable.
  2. Let the computer sleep.
  3. Resume from sleep.
  4. The app should NOT reload any tabs immediately.
  5. Connect the computer to a network.
  6. The app should reload all tabs.

Additional Notes
https://circleci.com/gh/yuya-oc/desktop/200#artifacts

@yuya-oc yuya-oc added this to the v3.7.0/3.8.0 milestone Mar 27, 2017
@jasonblais
Copy link
Contributor

@yuya-oc tested on Windows 10

After waking computer from sleep, each tab begins to reload

image

However, it seems the reloading process takes a really long time -- the page doesn't actually appear until I refresh the tab. Reproduced on the pre-release server and one of our test servers. I attached some console errors that might be relevant

image

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Mar 30, 2017

Thanks for catching! I missed that because badges was correctly updated.

I'm not sure the reason why this problem happens, but it's fixed by applying Electron's notes for webview on my Mac. Originally I worked on this at another branch though, I added it to this PR.

https://circleci.com/gh/yuya-oc/desktop/207#artifacts

@jasonblais
Copy link
Contributor

I seem to be hitting the same issue - the tabs do start to reload, but it seems to take a long time

image

Interestingly, the tab loads properly for test sites like example.com

image

and test.com

image

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Apr 1, 2017

Hmm...interesting. But I'm not sure. Perhaps it's too early to reload even when the computer is online in resuming.

And unfortunately I found another problem on my Windows 10 desktop. The reloading fails due to ERR_NETWORK_CHANGED error. Long (more than 5 seconds) delay was necessary to avoid it, so the delay might not comfortable.

Just curious, does mattermost/mattermost#5120 make sense for the original problem of #461?

@jasonblais
Copy link
Contributor

Yeah, mattermost/mattermost#5120 is related -- however the PR was included in Mattermost 3.6, and some people reported missing messages on that server.

However, we just had another PR merged (mattermost/mattermost#5907), which adds a sequence number to WebSocket connections and events -- that sequence is used to determine if any events were missed and run reconnect logic to get that missed data.

So I can check on Monday whether that PR would be sufficient to cover #461

@jasonblais
Copy link
Contributor

@yuya-oc chatted with Joram - although mattermost/mattermost#5907 won't be the perfect solution for resolving missing messages, it might be enough for this purpose. So propose keeping this PR as pending until Mattermost v3.8 is released and asking the original issuer to report back whether missing messages continue

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Apr 4, 2017

@jasonblais Okay. But the patch for webview seems necessary, so I will apply only it later.

@jasonblais
Copy link
Contributor

Thanks @yuya-oc, sounds good

@jasonblais jasonblais modified the milestones: v3.9.0/3.10.0, v3.7.0/3.8.0 Apr 5, 2017
@yuya-oc yuya-oc mentioned this pull request Apr 18, 2017
3 tasks
@yuya-oc yuya-oc mentioned this pull request Apr 29, 2017
3 tasks
@jasonblais
Copy link
Contributor

@yuya-oc Since Mattermost Server v3.8, there haven't been issues with missing messages. Hence, propose closing this PR as well as #461

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Jun 1, 2017

@jasonblais I agree. Thanks for feedback.

@jasonblais jasonblais closed this Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants