GNOME Bugzilla – Bug 160413
Photo ID support
Last modified: 2005-08-29 00:34:11 UTC
Photo ID display support should be added to properties widow. This bug is related with Debian bug #284155 (http://bugs.debian.org/284155)
This is yet another feature that GPGME doesn't support. As noted in bug# 159671 these features may need to just be implemented by us.
Werner Koch from the gpg-devel list has indicated that this functionality will be added to GPGME at some point. At that point we will enable it in Seahorse.
We can fake support for this feature by accessing the command line version of gpg. To add a photo: gpg --edit <keyID> addphoto You are then prompted for a JPEG file name and given a reccomended size: 240x288. KGPG does this with the following functions from kgpginterface.cpp: void KgpgInterface::KgpgAddPhoto(QString keyID,QString imagePath) void KgpgInterface::addphotoprocess(KProcIO *p) http://webcvs.kde.org/kdeutils/kgpg/kgpginterface.cpp?view=markup Similarly, *conprocess<< "gpg"<<"--no-tty"<<"--status-fd=2"<<"--command-fd=0"; *conprocess<<"--photo-viewer"<<QFile::encodeName(pgpgOutput)<<"--edit-key"<<userIDs<<"uid"<<QString::number(uid)<<"showphoto"; Can be used to get the photos into a file which can be read in and displayed.
Could I get someone to create a JPEG image of width 240 pixels and height 288 pixels? It's for the default case where no ID is present. It should maybe have a red question mark and the text Photo Unavailable.
Created attachment 37691 [details] Photo ID screen shot This is not a mockup! I created a dummy key and added a photoid manually. This means getting photo ids works at present. Next adding photoids.
Created attachment 37692 [details] [review] ROUGH patch showing what I did This is not meant to be applied to CVS but if anyone wants to give it a try they can. I'll attach my really ghetto no photo id jpeg below. You need to drop that in prefix/share/pixmaps/
Created attachment 37693 [details] GHETTO no photo id graphic
Comment on attachment 37692 [details] [review] ROUGH patch showing what I did I like the patch. I've looked over it roughly. I just have a bit of a problem with the modification of gpg.conf. I know we do it for the agent, but that's for a different purpose. Modifying it every time we retrieve a photo introduces all sorts of concurrency and interoperability issues (especially with other GPG key managers). Could we use the following approach instead: - gpg runs 'xloadimage' by default. - Create a small application helper (that we install into /usr/libexec/seahorse/ or something. This helper would be called 'xloadimage'. - The application helper reads data from stdin and outputs it to a file specified in the (for example) $SEAHORSE_PIC_TEMP environment variable. - Before running gpg_op_edit we prefix $PATH with the directory that the application helper is in, and set $SEAHORSE_PIC_TEMP to the location we want the pic written to. It sucks that we can't pass arguments to gpg via gpg_op_edit, but the above may be a more reliable work around than modifying gpg.conf.
That approach would work. I have no idea how to write the helper program at the moment though. I mentioned some problems with the current approach on the mailing list. Among them being that this only works well if there is only one photoid. Running show photo without "selecting" a uid on a key with multiple photoids results in all of them being displayed. In the case of editing gpg conf, the second id is output over the first. In the edit shell, it's possible to set a primary id, but I'm not sure what gpg does with it.
Created attachment 39054 [details] Helper Program A sample helper program. This would be compiled as 'xloadimage' and installed into (let's say) $prefix/libexec/seahorse/ That directory then gets added to the front of the path, etc.
After discussing this with Jim a bit, I've decided to put some more work into this feature. My current plans are to modify the "helper program" to append an index number on to the end of the temporary name in SEAHORSE_FILE_NAME. In this manner, we'll be able to support the primary photoid for now and able to implement support for secondary ids in the future. I think I'll have to borrow code from KGPG to delete photoids and determine which id we're working with since GPGME doesn't report the full number of id's.
Sounds good. Sad about GPGME not supporting the photo ids properly. I wonder if that's something we could patch in GPGME and then submit to the GPG project? But I'm not aware of what it entails exactly.
Created attachment 49660 [details] [review] Patch 1 of PhotoID support This is the first patch for implementing photoid support. It actually doesn't implement anything functionwise. It adds a field to SeahorseKey, revamps src/seahorse-key-properties.glade, and changes src/Makefile.am to build and install Nate's helper program. This patch requires the helper program to be saved as src/seahorse-xloadimage.c. I'd include screenshots of the new UI in action, but gnome-screenshot keeps crashing. More to come, same danger time same danger station.
Created attachment 49695 [details] [review] Latest Patch for photoid support This patch implements support for multiple photo ids, at most two. The commented out portion in src/seahorse-key-op.c:seahorse_key_op_photoid_get_list() needs to be fixed to handle any arbitrary number of photoids and to get rid of duplicate photoids when there is only one on the key. There is support for stepping back and forth between the keys in the UI. There seems to be a weird bug with when the previous button is set_sensitive(FALSE) and then TRUE in that the label portion remains greyed while the button is active. Finally, support for deleting uids is included. TODO: Add support for adding photo ids Signing uids should be almost trivial, but we need to discuss the UI.
Created attachment 49696 [details] seahorse-person.png This is pixmaps/seahorse-person.png.
Created attachment 49698 [details] [review] Fixes Weirdness This patch fixes the weirdness I was complaining about above. Still TODO: Add support for adding photoids
Created attachment 49801 [details] [review] Steal this Patch Now with gooey oh so good add photo id support. Czech it out so we can check it in. Elvis has left the building.
Don't forget to save the helper program as src/seahorse-xloadimage.c or seahorse-person.png as pixmaps/seahorse-person.png.
Created attachment 49816 [details] [review] Some spit and polish I found some things that needed to be made key pair only and did some clean up.
I've taken a look at this. A lot of nice work. I tried it out. It didn't actually work for me (see below) but I'm sure those issues will be resolved and we'll have working and stable photo support. MY ERRORS I got the following errors after adding a photo to a key and reopening seahorse: ** (lt-seahorse:23553): CRITICAL **: file seahorse-key-op.c: line 2382 (get_photoid_list_transit): should not be reached ** (lt-seahorse:23553): CRITICAL **: file seahorse-key-op.c: line 258 (edit_key): assertion `GPG_IS_OK (err)' failed seahorse_key_op_photoid_get_list threw error The key I added was flagged as 'too large' even after i made it smaller. Do you know what the maximum size should be? (See below for notes on cropping and sizing)... Perhaps this was the problem. GUI ISSUES - The photo are should integrate more properly with the key information. Perhaps other things could be taken away from the first key property page. Jim is working on a redesign of this dialog, and it would be good to get his input on this. Once we have different dialogs for personal and public keys, the delete and add buttons should not be visible in the case of others keys. - The next and previous buttons take up a lot of space. One idea it to display then ontop of the photo if there is more than one. These could be small arrow buttons, similar to the scrollbar end buttons but horizontal. The feel should be of flipping pages. Again, these buttons should only be visible on the image if more than one photo id is present (a small use case). PATCH SPECIFICS + gtk_file_filter_add_pattern (filter, "*.jpg"); + gtk_file_filter_add_pattern (filter, "*.jpeg"); We should generally only do this when the mime info is inadequate, as in the case of pgp keys with the .asc extension. + str = g_string_new (""); + g_string_printf (str, "gpg --list-keys 0x%s", seahorse_key_get_id... Could this code should be integrated into libseahorse/seahorse-gpgmex-op.c. It should probably use execute_gpg_command which properly figures out the gpg executable path and stuff. + gtk_widget_set_sensitive(glade_xml_get_widget (swidget->xml, "photoid-next"), + FALSE); + gtk_widget_set_sensitive(glade_xml_get_widget (swidget->xml, "photoid-prev"), + FALSE); + gtk_widget_set_sensitive(glade_xml_get_widget (swidget->xml, "photoid-delete"), + FALSE); We should keep state (ie: perhaps using g_object_set_data on the GtkImage). Next we make a function that using that state (ie: g_object_get_data) figures out the appropriate sensitive or visible state for these buttons. This is especially taking into account the above notes on hiding the buttons, or displaying the small scroller buttons on the image etc... The functions in seahorse-key-properties.c need some work to fall more into a 'state' type design, but once the above is implemented that should come natuarally. FUTURE ISSUES I'd commit without these but we need to implement them. We need to set ourselves apart from 'other' GPG applications by doing things that actually make GPG easier to use: - We should show a preview of the photo in the add photo dialog. - We should offer to resize and/or crop images that are the wrong size or orientation respectively. I'm not thinking of an actual graphical editor here, just an automatic thing. - We should integrate properly with the new 'About Me' capplet and settings which have picture selection. See: http://www.gnome.org/~davyd/gnome-2-12/images/about-me.png - Drag and drop. We should allow drag and drop to the picture area. - Scrollwheel support for scrolling through multiple images. If you like, we could break the above into seperate bugs once we have the basic support completed. Thanks again for all the hard work. It looks like it's coming together :)
There's a new patch that may clear up some of your bugs, Jim and I spent some time debugging this weekend. Jim found that adding another photo to the key caused the one that didn't show up to appear. I'm not having this problem, but does your test key have more than one non-photo id on it? I also found a reproducable bug in seahorse-daemon, double clicking on a jpeg in the file select dialog causes the passphrase entry to fail, but pressing cancel brings up the default passphrase query and everything works. If open is manually selected instead of double clicking, seahorse-daemon performs properly. I have not been able to trace the source of the bug. The line 2382 bug, Jim can reproduce, but I haven't been able to. As far as prettifying the UI, I assumed since Jim was going to be working on a complete retool that mine was more proof of concept. The preview in the open dialog isn't too hard to add, I saw how to do that in the API. But I'd say that's something to do after the base functionality has been added. Resizing/cropping might be a bit more difficult, gpg specifies a suggested pixel-by-pixel size, but I'm not sure what size triggers its size query. In addition to the about-me capplet which should probably pull a photo id off of the defualt key, evolution contacts should also pull photo ids from the various keys. Much of this is DBUS dependent though I would imagine since libseahorse isn't an external library. I like the scroll wheel and drag and drop functionality, but I think that's best put off until the UI rewrite is complete. The state stuff will probably require some research, but I'll try and address some of the stuff you mentioned and upload a new patch later today(read evening).
Created attachment 50102 [details] [review] same ol' same ol' This patch implement's some fixes to bugs discovered by Jim Pharis as well as moving the gpg command line to libseahorse/seahorse-gpgmex-op.c. I'm not sure what exactly how to implement states to control the buttons. Do you have to add a property to the GtkImage that keeps a pointer to a struct that contains the state information?
Jim and I tracked down why my patches weren't working for him and Nate. It's the gpg version used, I'm using 1.4.x and Jim's using 1.2.x. The output for gpg --list-keys <keyid> is different. We found that adding one to the number of uids found at line 116 of libseahorse/seahorse-gpgpmex-op.c if gpg 1.2.x was used fixed a number of things. Is there anyone that could hack our autoconf rules to set a GPG_VERSION_1_2 and/or GPG_VERSION_1_4 compile time definition?
Created attachment 50202 [details] [review] new Who's got a patch that should work for gpg 1.2.x and 1.4.x users? This guy. Who cleared up some weirdness with trying to add the same photo twice? This guy. You'll get a duplicate of the last photo id, but I think that will be taken care of with the MVC stuff.
Created attachment 50204 [details] [review] I lied Hopefully this will have the autoconf stuff right. You will have to do ./autogen.sh --prefix=<prefix> and make in the source root.
Created attachment 50338 [details] [review] Patch 10 With this patch, we're back to not doing much of anything due to Nate's commit generalizing things. This patch does have some things not in previous ones: * libseahorse/seahorse-gpgmex.h: added a struct gpgmex_photo_id_t similar to gpgme_user_id_t * libseahorse/seahorse-pgp-key.h and c: added functions to match uid functions TODO: * libseahorse/seahorse-pgp-key.h and c: add destructor for photoids * Implement MVC to make the photo ids work again
Created attachment 50364 [details] [review] Photo Patch 11 Adds deconstructor to for photo ids to seahorse-pgp-key
BTW, I'm merging this with the the big refactoring I've just added. Will post a patch shortly.
Created attachment 50666 [details] [review] Merged with HEAD, some updates I've updated your last patch so it works with what's currently in HEAD. I also pruned it a bit and took and made a few changes here and there. Hope that's okay. I couldn't test the patch as there's no UI included in the latest patch versions. So I hope I didn't break anything. Feel free to fix it back if I did. In case you're interested, notes on what I changed: - Moved seahorse-key-op.* to libseahorse. I've put in a request to CVS for the actual repository move. - seahorse-xloadimage.c should build with libseahorse. I'll move this later in the repository as well. - Updated seahorse-xloadimage so it doesn't use certain functions not found on Solaris and other wierd OS's. - Removed some unused functions, variables (configure with --enable-debug and you'll see them too). - Changed a few of the names of the structs and ops in seahorse-key-op.c to be slightly clearer. - When freeing a GdkPixbuf we need to g_object_unref it. g_free won't free internally used memory. - Cleaned up the formatting a little. - Added functions to seahorse-gpgmex for freeing gpgmex_photo_id_t - Uses g_stat g_unlink, g_mkstemp instead of their UNIX counterparts (for portability). - Added a SeahorseKey change type: SKEY_CHANGE_PHOTOS - Made it so that seahorse_key_op_photoid_load doesn't fire a "changed" event on the SeahorseKey. This is for when the photo ids change (let's say you add one).
Created attachment 50674 [details] [review] Added Debug Statements I applied this patch and started playing around with it. Before I added the UI stuff back, I wanted to make sure the code would at least run error free as is. However, it does not. It always seems to crash in the middle of seahorse_key_op_photoid_load. Output below shows proper operation with key 47141DAA20AF59F0 that has 1 UID and 1 Photo ID and then in a second instance of running seahorse crashing on processing the key. I haven't been able to replicate it, but one time the stderr popped up a Glibc warning about a corrupted doubly linked list. I'm not using any of those and haven't searched through SeahorseKeyOperation looking for them. I changed gpgmex_photo_id_free_all to use gpgmex_photo_id_free on each itteration of the loop instead of simply g_free and have it responsibly set the pointer to NULL after freeing the memory. This patch includes my modifications including debug statements. ***First Run*** $ seahorse ** Message: init gpgme version 1.0.2 [multi-operation 0x080CD350] start [multi-operation 0x080CD350] adding part: 0x080D3F98 [multi-operation 0x080CD350] single progress: (null) 1/1 [multi-operation 0x080CD350] adding part: 0x080EF700 [multi-operation 0x080CD350] multi progress: (null) 2/2 [multi-operation 0x080CD350] part complete (2): 0x080D3F98/(null) [multi-operation 0x080CD350] complete DNS-SD added: sadam's encryption keys hkp://192.168.1.101:33369 PhotoIDLoad Start PhotoIdLoad KeyID 47141DAA20AF59F0 PhotoIDLoad Number of UIDs 2 PhotoIDLoad Next UID 2 PhotoIDLoad Before set uid 2 PhotoIDLoad After set uid list->uid 2 PhotoIDLoad After image load PhotoIDLoad Done [multi-operation 0x080CD350] start [multi-operation 0x080CD350] adding part: 0x08223D88 [multi-operation 0x080CD350] single progress: (null) 1/1 [multi-operation 0x080CD350] adding part: 0x08240E50 [multi-operation 0x080CD350] multi progress: (null) 2/2 [multi-operation 0x080CD350] part complete (2): 0x08223D88/(null) [multi-operation 0x080CD350] complete PhotoIDLoad Start PhotoIdLoad KeyID 8D4D68687108E308 PhotoIDLoad Number of UIDs 2 PhotoIDLoad Next UID 2 PhotoIDLoad Done [multi-operation 0x080CD350] start [multi-operation 0x080CD350] adding part: 0x082688A8 [multi-operation 0x080CD350] single progress: (null) 1/1 [multi-operation 0x080CD350] adding part: 0x080D41D0 [multi-operation 0x080CD350] multi progress: (null) 2/2 [multi-operation 0x080CD350] part complete (2): 0x082688A8/(null) [multi-operation 0x080CD350] complete ***Seahorse Exits Cleanly*** ***Second Run*** $ seahorse ** Message: init gpgme version 1.0.2 [multi-operation 0x080CD358] start [multi-operation 0x080CD358] adding part: 0x080D3FA0 [multi-operation 0x080CD358] single progress: (null) 1/1 [multi-operation 0x080CD358] adding part: 0x080EF708 [multi-operation 0x080CD358] multi progress: (null) 2/2 [multi-operation 0x080CD358] part complete (2): 0x080D3FA0/(null) [multi-operation 0x080CD358] complete DNS-SD added: sadam's encryption keys hkp://192.168.1.101:33369 PhotoIDLoad Start PhotoIdLoad KeyID 48BBF0944DA565F9 PhotoIDLoad Number of UIDs 1 PhotoIDLoad Done [multi-operation 0x080CD358] start [multi-operation 0x080CD358] adding part: 0x0822BC90 [multi-operation 0x080CD358] single progress: (null) 1/1 [multi-operation 0x080CD358] adding part: 0x0823FCF8 [multi-operation 0x080CD358] multi progress: (null) 2/2 [multi-operation 0x080CD358] part complete (2): 0x0822BC90/(null) [multi-operation 0x080CD358] complete PhotoIDLoad Start PhotoIdLoad KeyID BE4374E7F9C2A165 PhotoIDLoad Number of UIDs 1 PhotoIDLoad Done [multi-operation 0x080CD358] start [multi-operation 0x080CD358] adding part: 0x08269228 [multi-operation 0x080CD358] single progress: (null) 1/1 [multi-operation 0x080CD358] adding part: 0x08246A90 [multi-operation 0x080CD358] multi progress: (null) 2/2 [multi-operation 0x080CD358] part complete (2): 0x08269228/(null) [multi-operation 0x080CD358] complete PhotoIDLoad Start PhotoIdLoad KeyID 47141DAA20AF59F0 PhotoIDLoad Number of UIDs 2 PhotoIDLoad Next UID 2 PhotoIDLoad Before set uid 2 ***Crash***
Created attachment 50705 [details] [review] Patch 13 UI Functions have been added, good lord the MVC model made these a lot easier to write. The same behavior from above occurs and some additional weirdness too. Somewhere between src/seahorse-key-op.c:2352 and src/seahorse-key-properties.c:814, the data stored in the gpgmex_photo_t structures is lost or messed with. The pixbuf is no longer a pixbuf or even a G_Object and the uid has garbage stored in it occasionally.
Created attachment 50737 [details] [review] Doesn't corrupt memory, Merged with HEAD Okay, I spent some time looking through all of this, and got it somewhat working. I merged this with the new file locations for seahorse-key-op.* which are now in libseahorse as seahorse-pgp-key-op.* FIXES: Not sure what this was supposed to do... + parm->first = *(g_new (gpgmex_photo_id_t, 1)); ...but it was overwriting all sorts of memory. It's a dereference on a newly allocated uninitialized pointer to a pointer. I added a function that allocates the structs to the gpgmex files and the patch now uses that. I also added a bunch of error checking in both seahorse-key-properties.c and seahorse-pgp-key-op.c. One of the things that was happening was that |stat| was failing and then we were using the uninitialized stat buffer, to check the file size. Another thing is that if the JPEG was in the wrong format (even some JPGs trigger this) then GPG would prompt for it again, which would hang. Added code to check for this. The photoid_delete_clicked and photoid_add_clicked now display any errors that occur when they're done. REMAINING A static global variable |current_photoid| is used to keep track of the current photo being displayed in seahorse-key-properties.c. This won't work when we have more than one 'Key Properties' window open. You need should make it window specific. You could use: g_object_set_data (G_OBJECT (swidget), "current-photoid", photoid); photoid = (gpgmex_photoid_t)g_object_get_data (G_OBJECT (swidget), "current-photoid"); Also when adding and deleting multiple photos things get out of sync. We need some more debugging in this area. We're getting close :) Should be ready for the initial commit soon.
Created attachment 50824 [details] [review] Patch 14 I think I found the bug that was giving the load list after deleting or adding a key problems. It turns out the load function really needs for pkey->photoids to be NULL and the free_all function wasn't doing it like I thought it was. Seems like we're almost passing a copy of the pointer in. I added code to set that to NULL after the free and some strict checking in the load function. While I was bug hunting there were also some changes to the set_photoid_state function to make the buttons work properly. Still ToDo: get rid of static current_photoid.
I changed it so we were passing the address of the pointer which to set to seahorse_key_op_load (or whatever that's called exactly) rather than it setting it right on the key. This is a more flexible approach.
Created attachment 50864 [details] [review] Patch 15 This may be the one. current_photoid is gone. The flexibility of passing the address of a random pointer to seahorse_key_op_photoid_load is back in. Test it. Be sure to really hammer on it. The only known bug occurs after deleting all photoids, import in the exported key pair and add a photoid. The key properties window goes ape. This may be cleared up with the code Nate will commit about reloading a loaded key after import.
Created attachment 51426 [details] [review] Synced with HEAD
I'd say we should go ahead and commit this, with the provisio that the interface needs to be reworked and any bugs fixed before we do our first 0.9.x release. Once this is committed we should file seperate bugs for each issue we see with it. Before committing could you go through and remove any commented out code, or out of place (non-debug-mode) printfs? Also, stuff like this here are preconditions: if (*photoids != NULL) { DEBUG_OPERATION(("photoids must be NULL\n")); g_assert(*photoids != NULL); } You should use: g_return_if_fail g_return_val_if_fail g_return_if_reached g_return_val_if_reached Another bug I noticed was if I added two photos in a row (to a key that initially had none), the second one would come be displayed as the little head until I closed the key props and opened it again. But again, once the above code nits are taken into account I think this is good to commit.