Comment 45 for bug 1506744

Revision history for this message
Allison Karlitskaya (desrt) wrote :

The patch in comment 42 is not acceptable, for two big reasons.

First: you need to hold a ref on the GFile object when you put it into the info struct. This means that you cannot simply use g_free for the struct: you need to write a custom free func that handles the unref as well. Also: you can't cast g_source_remove() to a GDestroyNotify and expect that to work... one takes a pointer, and the other an int. This may randomly work on some architectures, sometimes, but you cannot rely on this.

The entire act of keeping the list is slightly suspect. At the sprint I think I mentioned that it would be better to have one global timeout that is set (and reset) when any activity happens. The timeout would run after the activity died down, and it would handle all things together in one go. In that case, you'd still need a list, except for the info objects themselves. It's also fine to keep the list of the timeout IDs, but you can't free it in the way that you're doing in the current patch.

Another nice approach, if you want to avoid running the callbacks after teardown: store a weak pointer to the main object in each of the info structs (g_object_add_weak_pointer). The info structs are then used as the user_data to the timeout (this assumes you keep the multiple timeouts). If you find the weak pointer to be empty, then just return without doing anything. That also means that you don't need to handle the "cleanup" -- it will happen automatically. It also means that you can avoid the custom free function if you are clever: the timeout will always run once, and you can do the cleanup from the normal handler (taking care regarding the early exit discussed above).