Skip to content
This repository has been archived by the owner on Jan 23, 2018. It is now read-only.

Message Limit not imposed when tabbed away #72

Open
byronhulcher opened this issue Oct 16, 2014 · 11 comments · May be fixed by #76
Open

Message Limit not imposed when tabbed away #72

byronhulcher opened this issue Oct 16, 2014 · 11 comments · May be fixed by #76
Labels

Comments

@byronhulcher
Copy link
Contributor

I was tabbed away from chat.meatspac.es for around 10-15 minutes earlier tonight and realized my computer was starting to get toasty like when v2 first launched. Coming back to the page, I had 80+ (I stopped counting, in retrospect i probably could have jqueryed it) videos in total on the page. Seemed like it was also really slowing my computer down as well. Refreshing the page, and returning to the default 15 messages, everything sped up again, and now I'm typing this with my laptop on an ottoman waiting for it to cool down. Perhaps if messages are going to be added to the DOM while the tab is not active, then old messages should be popped as well? Not sure if that was a conscious decision to allow people to 'catch up'.

PS Before I alt-tabbed away, i was scrolled up to the top of the page. I'm not sure if that makes a difference when attempting to repro this, or if that might effect the pause/unpause behavior when in a different tab, but seemed like it could be important.

Chrome Version 38.0.2125.104
Mac OSX 10.9.5

@tec27
Copy link
Member

tec27 commented Oct 16, 2014

Not intentional if it's happening due to tab visibility. Are you sure you didn't have the browser scrolled up at all? When you're not scrolled to the bottom (and auto-scrolling doesn't happen) we keep all the messages around until you scroll back down. My hunch would be that you were scrolled up slightly, so it was keeping everything. Don't really see how that would happen with the code otherwise, as we simply count the number of lis in the messages list, and don't do anything based off tab visibility there.

@junosuarez
Copy link

I've experienced this too - in old and new meatspac. Maybe we could use the PageVisiblity api to keep track of new messages for favicon / title of background tabs but not insert them in the document

@imakewebthings
Copy link
Member

I think there's a 3-line fix that should take place anytime after the chat is appended.

if (document.hidden) {
  video.stop();
}

Currently there is a page visibility change event listener that starts and stops videos, but chats that are being added while tabbed away are playing (at least the ones "in view") are playing. I'm busy right now but can send this PR tonight if nobody else gets to it by then.

@tec27
Copy link
Member

tec27 commented Oct 16, 2014

Good find Caleb, that makes more sense to me :) That wouldn't result in it not obeying the chat limit, but could result in it having 30 videos playing, which would never happen if the tab were visible. Awesome!

Let this be a lesson to you all: never stop looking at meatspace.

@MylesBorins
Copy link
Member

This could potentially break stiching though : (

@tec27
Copy link
Member

tec27 commented Oct 16, 2014

That bridge was crossed when we started pausing offscreen videos

@MylesBorins
Copy link
Member

I was unaware that was being done… alas our cpus are worth more than our stitch=

@byronhulcher
Copy link
Contributor Author

I implemented @imakewebthings suggestion, but it appeared that if you were scrolled to the bottom, then new videos would be paused when you return to the window. He mentioned one might need to add a call to Waypoints.refreshAll() during the changeVisibility event callback in main.js, I haven't had a chance to test this though YMMV

@tec27
Copy link
Member

tec27 commented Oct 16, 2014

Seems like it shouldn't be necessary, unless the videos aren't getting marked as .in-view while the window is minimized (possible). Worth a try though

@imakewebthings
Copy link
Member

@byronhulcher I can't reproduce that pausing behavior using the three-liner alone. I'll submit the PR and let others see if they run into it.

@byronhulcher
Copy link
Contributor Author

Weird, I'm not able to replicate on my MB this morning.

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

Successfully merging a pull request may close this issue.

5 participants