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

Add exception check for missing Notify typelib #77

Closed
wants to merge 1 commit into from
Closed

Add exception check for missing Notify typelib #77

wants to merge 1 commit into from

Conversation

khardix
Copy link

@khardix khardix commented Sep 22, 2015

When running udiskie without installed libnotify and leaving default options for the daemon in place, the daemon's setup fails when it tries to load nonexistent gi.repository.Notify.

This patch adds a quick try/except check around the typelib loading, which should log warning about missing typelib and then continue the setup.

@coldfix
Copy link
Owner

coldfix commented Sep 22, 2015

Hey,

I'll think about this. I see your point that failing to start the application because of an optional feature isn't very sensible when using the default parameters. On the other hand failing seems just the right thing to do, when udiskie is started with explicit request for notifications. I can see the following alternative solutions:

  • just improve error message when failing due to missing typelib (what went wrong? what can be done about it?)
  • fail if notifications are requested explicitly on the command line, just warn if using the default
  • disable notifications by default (which I think should have been the default from the start, but now it's too late for that..)

@khardix
Copy link
Author

khardix commented Sep 22, 2015

Hi,

I agree this quick&dirty fix would cause confusion when explicitly asking for notifications without having libnotify - which sucks.

As for your proposals, the quickest way would be to improve error message AND disable notifications by default - since not installing an optional component and then getting an error about that component kind of makes the component not so optional :( However, I dislike the idea to turn notifications off by default, as a new user than can miss this feature quite easily.

The best solution seems to be to fail if the notifications are requested explicitly, and only issue warning when used by default. But I suspect that the detection of implicit/explicit options would not be easy.

I would let you decide the course of action. I can even look into the implicit/explicit detection, but I don't know when I will get the time.

@coldfix
Copy link
Owner

coldfix commented Sep 22, 2015

It should be quite easy to distinguish explicit vs. implicit. Best you let me worry about the code, since I'm much more familiar with the code base and probably can do it much faster;)

@khardix
Copy link
Author

khardix commented Sep 23, 2015

OK then, I will close this PR and add this as issue. Let me know if you would use any help.

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.

None yet

2 participants