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

feat: update peer stats only when profile window is open #283

Closed
wants to merge 1 commit into from

Conversation

swang
Copy link

@swang swang commented Jun 4, 2016

Please let me know if my assumptions about things were correct (like shouldPoll being unnecessary if we're triggering from the 'show' event.)


Polling for the number of peers is now only updated when the user opens
the profile window rather than having a timer constantly updating in the
background.

Got rid of shouldPoll because it seems unnecessary with the change in
actually shown.

Added a new event handler to trigger the polling when the window is
how timers are set. The application also doesn't immediately run
pollStats when loading the ipfs daemon.

Closes #60.

Polling for the number of peers is now only updated when the user opens
the profile window rather than having a timer constantly updating in the
background.

Got rid of `shouldPoll` because it seems unnecessary with the change in
actually shown.

Added a new event handler to trigger the polling when the window is
how timers are set. The application also doesn't immediately run
`pollStats` when loading the ipfs daemon.
@swang
Copy link
Author

swang commented Oct 7, 2016

i know there is a conflict here from the recent merges but should i bother fixing this? this pr has sat here for ~4 months without nary a comment. is it okay? does it need changes?

i understand people are busy so i'm not pushing for an immediate merge. just want to know if i should spend time to fix the conflict.

@dignifiedquire
Copy link
Member

So sorry about the lack of feedback, let me review it now

@@ -240,6 +231,8 @@ export function boot (lokker) {
// Ensure single instance
mb.app.makeSingleInstance(reboot)

mb.on('show', () => { pollStats(getIPFS()) })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be once to ensure it is only started once

const statsCache = {}

function pollStats (ipfs) {
const next = () => {
setTimeout(() => {
pollStats(ipfs)
if (mb.window.isVisible()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check for mb.window I have seen it being undefined in edge cases

@dignifiedquire
Copy link
Member

Looks pretty good to me, left some notes. If you could rebase & fix those I can merge it in

@swang
Copy link
Author

swang commented Oct 11, 2016

hey so i tried running the latest master and was getting some errors. i tried reinstalling and now i just get a blue screen with a loading circle in the middle. are you seeing this as well?

@swang
Copy link
Author

swang commented Oct 11, 2016

so i drilled down and found out that in ipfsd-ctl it makes subcalls to go-ipfs and go-ipfs is complaining about a repo/package that is not up to date. this message gets drowned out in startDaemon and thus never propagates up to the surface.

this is what i see when i try to run ./ipfs daemon

Initializing daemon...
Adjusting current ulimit to 1024...
Successfully raised file descriptor limit to 1024.
Found outdated fs-repo, migrations need to be run.
Run migrations now? [y/N]

@victorb
Copy link
Member

victorb commented Oct 11, 2016

Try clearing (removing) your $IPFS_PATH folder or migrate it manually
with https://github.com/ipfs/fs-repo-migrations

@daviddias
Copy link
Member

@hacdias are you taking this into account with the Redesign PR #516 ?

@hacdias
Copy link
Member

hacdias commented Dec 2, 2017

No, but I will. @diasdavid

@hacdias hacdias mentioned this pull request Dec 2, 2017
20 tasks
@daviddias
Copy link
Member

This was already integrated, see comment here #516 (comment). Thanks for the PR either way @swang :)

@daviddias daviddias closed this Dec 2, 2017
@swang
Copy link
Author

swang commented Dec 2, 2017

The issue wasn't that polling didn't activate when you opened the window. The issue was that it continued to poll once you closed the window.

You can look at the OP of this PR and the diff in the code and also look at the comments in #60.

@hacdias
Copy link
Member

hacdias commented Dec 2, 2017

If you look at this line it will not poll if the window is closed. Yes, it will call the function again, but the API of IPFS never gets called. Unless you don't even want to call the polling function while the window is hidden. In that case, we could simply use events to start and stop polling.

@swang
Copy link
Author

swang commented Dec 2, 2017

I guess I must have been confused then. I vaguely remember checking if it made the api calls beforehand to see if they fired after the window closed.

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.

5 participants