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 QSocketNotifier error on startup #5102

Merged
merged 2 commits into from
Sep 2, 2017

Conversation

nyalldawson
Copy link
Collaborator

And avoid unnecessary creation of unused QFileSystemWatchers

*
* If \a watchProfileFolder is true, then the specified \a rootLocation will be watched and any newly added
* profiles will cause profilesChanged() to be emitted. If \a watchProfileFolder is false, then the this will
* not occur.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a very brief explanation of why you would (not) want the folder to be watched?

@NathanW2
Copy link
Member

NathanW2 commented Sep 2, 2017

When did you get a socket error?

@nyalldawson
Copy link
Collaborator Author

When did you get a socket error?

You get one immediately on startup: "src/app/main.cpp: 450: (main) [0ms] Starting qgis main
Warning: QSocketNotifier: Can only be used with threads started with QThread"

It's because the watcher is created before QgsApplication.

@nyalldawson
Copy link
Collaborator Author

I'm going to tweak this PR - I don't like the extra argument in the constructor.

And avoid unnecessary creation of unused QFileSystemWatchers
@nyalldawson
Copy link
Collaborator Author

Ok, done. This is a much cleaner approach which makes the new profile watcher (and its notifications) explicitly opt-in.

@m-kuhn

Can you add a very brief explanation of why you would (not) want the folder to be watched?

Short lived instances of the manager are sometimes created just to handle resolving paths for profiles. In this case the profile addition notification signals aren't being used/wanted, so it's just extra overhead. I think the new approach is more self-explanatory, so let me know if you think the documentation is sufficient.

@NathanW2
Copy link
Member

NathanW2 commented Sep 2, 2017

Thanks. That looks good.

@m-kuhn
Copy link
Member

m-kuhn commented Sep 2, 2017

Looks good to me now.

%Docstring
Emitted when the list of profiles is changed.

This signal will only be emitted when isNewProfileNotificationEnabled() is true.
By default By default new profile notification is disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

By default By default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No sorry, as a native English speaker "by default by default" reads much better than "by default"....

... ok, I lie.

@nyalldawson nyalldawson merged commit 8e12757 into qgis:master Sep 2, 2017
@nyalldawson nyalldawson deleted the socket_error branch September 2, 2017 06:13
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.

4 participants