mate-desktop / mate-sensors-applet

Display readings from hardware sensors in your MATE panel
http://www.mate-desktop.org
GNU General Public License v2.0
16 stars 13 forks source link

applet doesn't update sensorlist if a drive is removed from system #62

Closed raveit65 closed 6 years ago

raveit65 commented 6 years ago

Expected behaviour

Sensor-list in preferences should be updated if a drive doesn't exists any more.

Actual behaviour

Sensor-list shows non-existent drives in preferences and if this sensor is enable we see a lot of popup warnings.

Steps to reproduce the behaviour

  1. plug in an USB-drive
  2. reload panel or applet
  3. activate the temp sensor of this drive in applet
  4. unmount and unplug usb-drive
  5. reload applet or panel
  6. enjoy frequently popup warnings

MATE general version

master branch, 1.19.x

Package version

master branch, 1.19.1

Linux Distribution

fedora 26 with Mate developer version

Link to downstream report of your Distribution

not relevant

Problem exists since we use a gsettings key to store sensor order. https://github.com/mate-desktop/mate-sensors-applet/commit/c5bce79ac7c654716cd8e1d69882a67053d469ef

The applet should be check for existing sensors with every start to update sensor-list, or better a frequently check for catching new pluged in usb-, e-sata- or other drives. @info-cppsp Can you please take a look at this?

info-cppsp commented 6 years ago

Right. I didn't think of that. Sure, let me check.

info-cppsp commented 6 years ago

https://github.com/mate-desktop/mate-sensors-applet/issues/56#issuecomment-346873336 If you look at this, you'll see that this is a logistics problem. Before, the plugins searched for the sensors first, now, the sensors are loaded from gsettings first and that is why any sensors that have been saved at msa end (like when restarting the computer) are loaded again. Now If a loaded sensor is no longer in the system, I imagine, that there are some warnings.

BUT obviously unless you enable a sensor from a removable device (3. activate the temp sensor of this drive in applet), this is not a problem. Therefore this is more like a feature request.

I have a WD My passport USB HDD. Can't set it up in msa. UDisks2 doesn't show smart data for it...

Possible solutions. 1. Load sensors normally. (Load them from gsettings. - not needed) Do separate sorting.

2. Load them from gsettings. (to get the sorting) Load sensors normally, mark found sensors. (Something equivalent to increase reference count.) Throw away all sensors that haven't been found normally. (They are no longer there.)

Both of these are kinda messy, so I haven't done them.

Essentially I need a way to sort the sensors_applet->sensors GtkTreeStore with an array of hashes. I don't know how to do that. :(

raveit65 commented 6 years ago

Well, disabling the sensor in preferences will stop the popup warnings. But the only way to remove the sensor from preferences, is to remove the very cryptic hash number from gsettings key by hand. And you need to know the hash number from the removed usb-drive. I think for testing you can add a dummy hash number to gsettings key, this should trigger the warnings......maybe.

monsta commented 6 years ago

I didn't know about this. I think I don't have any removable drives with sensors though...

monsta commented 6 years ago

While testing fan sensor on eeepc, I reproduced this when I removed the module (sudo rmmod eeepc-laptop). The fan sensor disappeared, but it was enabled in the applet, and a lot of warning notifications followed. That's with version 1.18.3 though.

info-cppsp commented 6 years ago

^ v. 1.18.3 has the same PR as well.

@raveit65

  1. Please try an older msa version (before the PR) as well.
  2. Please make a video. (So I can actually see what happens.)

I have made the following test:

  1. Turn off PC.
  2. Add new HDD.
  3. Turn on PC, set msa prefs to show temp of new HDD.
  4. Turn off PC.
  5. Remove new HDD.
  6. Turn on PC.

I got the following on the mate-panel: screenshot at 2018-01-18 22-28-38

That is acceptable - I think. Even if I suspend and remove the HDD, I still don't get popups.

One could change the if in active-sensor.c active_sensor_update() line 552 if (error) { and add some code to not display anything on error even if a sensor is set in the preferences. I don't think, that that is a good idea though.


I am not really sure what the issue is. a, instead of a value, msa shows ERROR, if the sensor is no longer there OR b, if a sensor is removed, msa shows popups (I can't reproduce this.) OR c, as in the title "msa doesn't update sensorlist if a sensor is removed from system" -> how does msa know if the sensor is actually removed, not just throwing an error??

A solution would be to add another property to a sensor, like a gboolean 'removable', and if this is set, turn off the error handling for the sensor. The sensor could be loaded on msa start, but not be active on the mate-panel, until the drive gets attached.


@raveit65 Why do you reload the applet in 2. and 5.? I don't think that is a normal usage, right?

Look at what @monsta said. 'I reproduced this when I removed the module' I think no one would be surprised that removing a kernel module on-the-go breaks things. -.-


SO from my point of view, this is a very specific issue. I can't really help with it, bc I can't reproduce it. :( Please give me more information.

Otherwise I would say, that we should just forget about this. In my test, if I remove a drive (may not be the same for other sensors) I only get the 'ERROR' on the mate-panel. If I attach the drive again, msa shows the temp again as normal. Maybe we could set hide on error for that activ-sensor, but then, how would you know if there is an error with a sensor...

info-cppsp commented 6 years ago

I didn't have libnotify-dev... .......

now I get the annoying popups yay

info-cppsp commented 6 years ago

how about this? I make an error_timestamp property for active-sensors. I set this only on SENSOR_INTERFACE_ERROR. If the last error was before current time - 11 sec, then show the error.

This should stop the infinite loop of popups. Better ideas?

raveit65 commented 6 years ago

Thanks for looking into it. I will compare m-s-a version with what i ship for rhel7/centos7 1.16 with gtk2 https://koji.fedoraproject.org/koji/buildinfo?buildID=838606 1.18 with gtk3 https://copr.fedorainfracloud.org/coprs/raveit65/Mate-GTK3/build/607961/ I can't remember that that i run into this problem before 1.18.3 but i am sure someone in the world who use MATE had reported it as using usb-drives isn't special.

how about this? I make an error_timestamp property for active-sensors. I set this only on SENSOR_INTERFACE_ERROR. If the last error was before current time - 11 sec, then show the error.

Great idea to stop the endless popups after a while, should be sufficient as first fix.

With that and in an ideal world m-s-a should load from gsettings and check for existing sensors after load again to detect non existing sensors. This should be done frequently to detect unplugged devices. No idea if this is possible or am i a dreamer ?

Another problem is the cryptic device hash which you should know to delete the sensor from gsettings key. Maybe you can offer a solution to delete a non existing sensor from GUI?

Sorry that i can't help more with coding for myself....

info-cppsp commented 6 years ago

I think I got rid of the popup problem.

The problem left: How do I know if a drive is removed from system? Maybe use udisk2 GDBus - but that is just one plugin. Even if it is, what should msa do? a, keep displaying ERROR b, hide display c, throw away sensor (even from GSettings??)

Should there be a way at all to keep removable devices / sensors in msa? If yes, then there needs to be a way to remove them. (Like another button 'Remove' on the sensors tab.) If no, then maybe the better solution would be to do the sorting...

I am not sure about the hash. What else is unique? Maybe path, but that is also cryptic.

raveit65 commented 6 years ago

I re-tested it with rhel7.4 and m-s-a-1.16.1 (gtk2) Here you can unplug an usb-drive + restarting the applet/panel and you don't get any warning about an missing drive. Of course the drive isn't in preferences any more.

Without making it more complicated, a removed disks should simply removed from preferences and gsettings if possible. So , first a warning should be displayed and after a while the disk should be removed. If the same disk will pluged in agan than m-s-a will read it after a restart again. A runtime detection/un-detection is another step and more luxury, no need to add this now , imo. The important thing should be to find a way for a automatic removal or to make it more easier for the user himself to remove the drive.

Maybe use udisk2 GDBus - but that is just one plugin.

I think the main problem still exists only with disks, other sensor like mobo or CPU won't remove normally. Maybe a user can change a graphic card? Edit: As most user use notebooks nowadays, a graphic card change isn't a common use case, imo

raveit65 commented 6 years ago

Re-opened as fix is only a first aid for solving the problem.

info-cppsp commented 6 years ago

You didn't have to do that, but ok. I still have it on my list, so no worries. ;-)

raveit65 commented 6 years ago

Any progress?

info-cppsp commented 6 years ago

well, lately I have been playing with mp and a game. ;-) Option a, add a global flag to sensors-applet.c, so that when the sensors are loaded from gsettings, flag = TRUE. set flag to FALSE. add sensors normally, during which the sensors that are already loaded, and would be added from the plugins as well, would be duplicate, set flag = FALSE write a function that iterates through the sensor tree and removes sensors, where flag = TRUE. (the sensors loaded from gsettings, but not from the plugins.)

This seems easy enough, but means: loading sensors from gsettings loading sensors from plugins removing them

Option b, get list of hashes from gsettings. sort gtk_tree_store with list. (Still not clear how to do that exactly...)

This means the sorting, but otherwise a cleaner solution. loading sensors from plugins (like before) sorting

Ideas on sorting a gtk_tree_store?

info-cppsp commented 6 years ago

fixed (again) with #77