HarlemSquirrel / gnome-shell-extension-sensory-perception

Displays CPU/GPU/mobo/disk temperatures, voltages and fan speeds
https://extensions.gnome.org/extension/1145/sensory-perception/
GNU General Public License v3.0
27 stars 11 forks source link

Fix Missing Displays for Disabled Devices #2

Closed Queuecumber closed 6 years ago

Queuecumber commented 7 years ago

I noticed that when I had my WiFi adapter turned off, all devices would be missing from the extension status and it would be telling me to re-run sensors-detect even though the command line outputs just fine. I traced this to a premature closing of the stderr stream from the child process causing all the childs fds to get cleaned up.

When a device is disabled, sensors spits up an error while reading it, and writes this to stderr while also writing "N/A" for the temperature reading on stdout. It seems that for whatever reason, if stderr is used by the child process, it needs to be closed after we are completely done reading from the child process or it will also close stdout causing the extension to think that nothing was output by sensors and that no devices have been configured.

This patch corrects the problem by storing stderr and closing it once we are done with stdout. I also tried using the SpawnFlags.STDERR_TO_DEV_NULL which, in my mind, is the better way to handle this situation, but it seems like the javascript bindings are not hooked up correctly because no matter what I tried I had a failed assertion.

With the patch applied, the disabled device will simply disappear from the list of devices shown in the extension. I toyed with other ideas, such as displaying the N/A, or displaying something like "Off" or "Disabled" for that device but I decided to keep it as simple as possible pending feedback from the developers. If you would like some kind of display like that, rather than the device simply not showing when it is disabled, let me know and I can submit it in another PR.

HarlemSquirrel commented 6 years ago

@Queuecumber Sorry about the delay I actually forgot this was open. It looks good to me. Thanks for fixing this! I'm going to merge now and try it out on my machine.