After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 160413 - Photo ID support
Photo ID support
Status: RESOLVED FIXED
Product: seahorse
Classification: Applications
Component: general
0.7.x
Other Linux
: Low enhancement
: ---
Assigned To: Seahorse Maintainer
Seahorse Maintainer
Depends on:
Blocks:
 
 
Reported: 2004-12-04 09:24 UTC by José Carlos García Sogo
Modified: 2005-08-29 00:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Photo ID screen shot (115.43 KB, image/png)
2005-02-20 01:33 UTC, Adam Schreiber
  Details
ROUGH patch showing what I did (65.76 KB, patch)
2005-02-20 01:49 UTC, Adam Schreiber
needs-work Details | Review
GHETTO no photo id graphic (10.39 KB, image/jpeg)
2005-02-20 01:50 UTC, Adam Schreiber
  Details
Helper Program (668 bytes, text/x-csrc)
2005-03-22 05:36 UTC, Stef Walter
  Details
Patch 1 of PhotoID support (85.09 KB, patch)
2005-07-24 03:46 UTC, Adam Schreiber
none Details | Review
Latest Patch for photoid support (109.32 KB, patch)
2005-07-25 01:22 UTC, Adam Schreiber
none Details | Review
seahorse-person.png (2.23 KB, image/png)
2005-07-25 01:45 UTC, Adam Schreiber
  Details
Fixes Weirdness (108.58 KB, patch)
2005-07-25 04:46 UTC, Adam Schreiber
none Details | Review
Steal this Patch (121.11 KB, patch)
2005-07-26 20:54 UTC, Adam Schreiber
none Details | Review
Some spit and polish (122.00 KB, patch)
2005-07-27 01:06 UTC, Adam Schreiber
needs-work Details | Review
same ol' same ol' (123.26 KB, patch)
2005-08-02 03:03 UTC, Adam Schreiber
none Details | Review
new (124.93 KB, patch)
2005-08-04 02:08 UTC, Adam Schreiber
none Details | Review
I lied (125.19 KB, patch)
2005-08-04 03:35 UTC, Adam Schreiber
none Details | Review
Patch 10 (110.71 KB, patch)
2005-08-07 00:06 UTC, Adam Schreiber
none Details | Review
Photo Patch 11 (111.40 KB, patch)
2005-08-08 02:16 UTC, Adam Schreiber
none Details | Review
Merged with HEAD, some updates (118.00 KB, patch)
2005-08-13 22:54 UTC, Stef Walter
none Details | Review
Added Debug Statements (117.72 KB, patch)
2005-08-14 04:35 UTC, Adam Schreiber
none Details | Review
Patch 13 (125.75 KB, patch)
2005-08-14 23:51 UTC, Adam Schreiber
none Details | Review
Doesn't corrupt memory, Merged with HEAD (125.14 KB, patch)
2005-08-15 20:08 UTC, Stef Walter
none Details | Review
Patch 14 (124.76 KB, patch)
2005-08-17 02:52 UTC, Adam Schreiber
none Details | Review
Patch 15 (125.28 KB, patch)
2005-08-17 17:22 UTC, Adam Schreiber
none Details | Review
Synced with HEAD (125.35 KB, patch)
2005-08-27 16:38 UTC, Adam Schreiber
committed Details | Review

Description José Carlos García Sogo 2004-12-04 09:24:18 UTC
Photo ID display support should be added to properties widow.

This bug is related with Debian bug #284155
(http://bugs.debian.org/284155)
Comment 1 Stef Walter 2004-12-06 23:45:48 UTC
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.
Comment 2 Adam Schreiber 2004-12-20 03:48:31 UTC
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.
Comment 3 Adam Schreiber 2005-02-07 00:30:49 UTC
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. 
Comment 4 Adam Schreiber 2005-02-19 18:30:01 UTC
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.
Comment 5 Adam Schreiber 2005-02-20 01:33:32 UTC
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.
Comment 6 Adam Schreiber 2005-02-20 01:49:35 UTC
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/
Comment 7 Adam Schreiber 2005-02-20 01:50:14 UTC
Created attachment 37693 [details]
GHETTO no photo id graphic
Comment 8 Stef Walter 2005-03-04 17:24:30 UTC
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.
Comment 9 Adam Schreiber 2005-03-04 18:11:54 UTC
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.
Comment 10 Stef Walter 2005-03-22 05:36:09 UTC
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.
Comment 11 Adam Schreiber 2005-07-15 03:24:11 UTC
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.
Comment 12 Stef Walter 2005-07-18 16:56:55 UTC
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.
Comment 13 Adam Schreiber 2005-07-24 03:46:25 UTC
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.
Comment 14 Adam Schreiber 2005-07-25 01:22:00 UTC
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.
Comment 15 Adam Schreiber 2005-07-25 01:45:54 UTC
Created attachment 49696 [details]
seahorse-person.png

This is pixmaps/seahorse-person.png.
Comment 16 Adam Schreiber 2005-07-25 04:46:30 UTC
Created attachment 49698 [details] [review]
Fixes Weirdness

This patch fixes the weirdness I was complaining about above.

Still TODO: Add support for adding photoids
Comment 17 Adam Schreiber 2005-07-26 20:54:18 UTC
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.
Comment 18 Adam Schreiber 2005-07-26 20:55:16 UTC
Don't forget to save the helper program as src/seahorse-xloadimage.c or
seahorse-person.png as pixmaps/seahorse-person.png.
Comment 19 Adam Schreiber 2005-07-27 01:06:23 UTC
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.
Comment 20 Stef Walter 2005-08-01 17:02:10 UTC
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 :)

Comment 21 Adam Schreiber 2005-08-01 18:07:55 UTC
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).
Comment 22 Adam Schreiber 2005-08-02 03:03:27 UTC
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?
Comment 23 Adam Schreiber 2005-08-03 03:11:35 UTC
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?
Comment 24 Adam Schreiber 2005-08-04 02:08:27 UTC
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.
Comment 25 Adam Schreiber 2005-08-04 03:35:42 UTC
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.
Comment 26 Adam Schreiber 2005-08-07 00:06:12 UTC
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
Comment 27 Adam Schreiber 2005-08-08 02:16:36 UTC
Created attachment 50364 [details] [review]
Photo Patch 11

Adds deconstructor to for photo ids to seahorse-pgp-key
Comment 28 Stef Walter 2005-08-13 21:08:10 UTC
BTW, I'm merging this with the the big refactoring I've just added. Will post a
patch shortly.
Comment 29 Stef Walter 2005-08-13 22:54:37 UTC
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).
Comment 30 Adam Schreiber 2005-08-14 04:35:49 UTC
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***
Comment 31 Adam Schreiber 2005-08-14 23:51:40 UTC
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.
Comment 32 Stef Walter 2005-08-15 20:08:03 UTC
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.
Comment 33 Adam Schreiber 2005-08-17 02:52:52 UTC
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.
Comment 34 Stef Walter 2005-08-17 03:32:06 UTC
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. 
Comment 35 Adam Schreiber 2005-08-17 17:22:45 UTC
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.
Comment 36 Adam Schreiber 2005-08-27 16:38:47 UTC
Created attachment 51426 [details] [review]
Synced with HEAD
Comment 37 Stef Walter 2005-08-28 22:03:53 UTC
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.