search

andrew-bibb / cmst

QT GUI for Connman
174 stars 37 forks source link

Only call connectNotityClient when needed #187

Closed nicklaswj closed 5 years ago

nicklaswj commented 6 years ago

Only call connectNotitfy when the setting "enable_daemon_notifications" is true, to avoid unnecessary DBUS timeout wait time on CMST launch

nicklaswj commented 6 years ago

I don't have a notify daemon installed which means that without this change it takes 4*dbus-timeout for cmst to be usable, which is annoying.

Otherwise thanks for a great piece of open source software !

andrew-bibb commented 6 years ago

Yes you are correct, we should not go through the four timeouts if someone has said don't use the notification client. I would like to review the current code before I merge this. For consistency I may want to handle this the same way we disable the VPN manager if somebody does not want it. It may take me a day or two (or three or four...) before I see which way I want to go. Either way this feature will be incorporated.

A quick look through just now and I also found one other initialization that I had which should be handled in a more consistent manner with the rest of the code.

Thank you for finding it and preparing the PR.

nicklaswj commented 6 years ago

Thanks :)

nicklaswj commented 6 years ago

If you have a clear idea how this should be handled, but don't currently have the time to implement it, I'll be more than willing to give it try

andrew-bibb commented 6 years ago

Thanks. I may just merge this PR. Really haven't had time to look at anything more in depth. I've got (or had) 3 cords of firewood piled in my driveway that has to be moved and stacked. Down to about 3/4 of a cord left. Once that is done my time will free up somewhat. As a completely unrelated aside, it stinks to get old. 35 years ago I could move that amount of wood in a couple of nights. It now takes a couple of weeks.

nicklaswj commented 6 years ago

Haha, if it helps; I'm 26 and wouldn't be able to do it 😊

I'm getting less and less happy with my own PR. One problem I've noticed with my PR is, if you have a notification daemon but have unchecked the notification daemon setting, then it doesn't show which notification daemon it has detected ( because my code disables the detection). It's not a big problem it's just not a very polished solution.

How do you feel about (a small amount of) asynchronous code? I could maybe handle the vpn stuff too, but I haven't looked into it yet.

lør. 23. jun. 2018 16.13 skrev andrew-bibb notifications@github.com:

Thanks. I may just merge this PR. Really haven't had time to look at anything more in depth. I've got (or had) 3 cords of firewood piled in my driveway that has to be moved and stacked. Down to about 3/4 of a cord left. Once that is done my time will free up somewhat. As a completely unrelated aside, it stinks to get old. 35 years ago I could move that amount of wood in a couple of nights. It now takes a couple of weeks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/andrew-bibb/cmst/pull/187#issuecomment-399681133, or mute the thread https://github.com/notifications/unsubscribe-auth/ABX51JK1I_1KohdANb6AXwsLjEMc9kDKks5t_kzugaJpZM4UqjEV .

andrew-bibb commented 6 years ago

Just finished with the wood, but I'm exhausted. I looked at what I was thinking and it is not immediately clear I can do what I want (and said exhaustion is not helping). I've been using the isValid() function in QDbusInterface for other similar checks, and I might be able to do that as there is an QDbusInterface member of the notify class. On the other hand it may not be worth the effort and you already have a working solution.

I have no problem with not showing which notification daemon has been detected if the user has disabled notifications. Really not much reason to show it in that case. Asynchronous code is fine, there is a lot already in there with some of the DBus replies. Where exactly would you be using it?

If you want to polish this one up a bit that is fine, but I think right now I'm leaning towards adopting it (as soon as I go over it carefully).

andrew-bibb commented 5 years ago

Thank you and sorry this took so long, my year kind of fell apart. With any luck 2019 will be a bit calmer.