GNOME Bugzilla – Bug 341772
Deleting a single key fails
Last modified: 2006-05-16 14:49:42 UTC
Jim Pharis identified a bug whereby deleting multiple keys worked, but deleting single keys didn't. He tracked down the error to libseahorse/seahorse-key-store.c:seahorse_key_store_get_selected_key() not setting the uid passed in correctly. He determined that replacing instances of seahorse_key_store_get_selected_key() with seahorse_key_store_get_selected_keys() solves the problem. Based on his work I will attach a patch that does that. However, we should continue to investigate why gtk_tree_model_get (model, iter, KEY_STORE_UID, &uid, -1); in key_from_iterator isn't setting the correct uid. Although when the key isn't a sub key, it should probably be set to 0.
Created attachment 65457 [details] [review] Patch based on Jim Pharis' work
Does seahorse_key_store_get_selected_key not work? If not it should probably be fixed rather than putting work arounds all throughout the code. Also this patch removes the ability to delete a single UID from the key manager.
I think Jim is looking at whether the model contains the correct data. I've only tested the bug against keys with 1 UID, the main one. While it does remove that ability, should that level of granularity be present in the key manager?
Nate, seahorse_key_store_get_selected_key takes the uid as a param, its currently broken in seahorse-key-manager because the caller doesn't set uid. The seahorse_key_store_get_selected_key may or may not work, but either way I don't see the need for it when seahorse_key_store_get_selected_keys does, from what I can tell, the same thing, but without the need for the uid to be passed. That function ..._keys finds the uid from the view, and works correctly, even when needing to find a single key. Its like there is an entire extra function just for the single use case of finding 1 key. - Jim P.S. I ran through some model appends and it appears the correct data is getting into the model. I think it changing afterwards somehow (but before the view is displayed). I'm still looking.
(In reply to comment #3) > I think Jim is looking at whether the model contains the correct data. I've > only tested the bug against keys with 1 UID, the main one. While it does > remove that ability, should that level of granularity be present in the key > manager? I think so. It makes sense to the user. "I select this row. I select delete. This row should go away." The principle of least suprise. (In reply to comment #4) > seahorse_key_store_get_selected_key takes the uid as a param, its currently > broken in seahorse-key-manager because the caller doesn't set uid. |uid| is an 'out' argument. You pass a guint* in and it returns the uid. If you pass in NULL it ignores that argument. > The seahorse_key_store_get_selected_key may or may not work, but either way I > don't see the need for it when seahorse_key_store_get_selected_keys does, from > what I can tell, the same thing, but without the need for the uid to be passed. Because as noted above, we like to only delete a single UID if that's what is selected. > That function ..._keys finds the uid from the view, and works correctly, even > when needing to find a single key. Its like there is an entire extra function > just for the single use case of finding 1 key. You'll find this all throughout seahorse. This prevents us from allocating a whole list of keys, and then freeing that list. In many cases it is worth it.
Created attachment 65594 [details] [review] This patch fixed the problem. It was a simple (but dumb) mistake in my recent refactoring... Committed.