Created attachment 27253 [details]
edit-keyring screenshot attempt #1

Attached is a proposed UI screenshot for the edit keyring window. It was
created using glade-2.6.0. I have tried to follow the HIG, but I am new at
this.

Users can edit the value by clicking on it. The screenshot shows the user
editing 'Port'. The cursor doesn't appear in the screenshot.

The raw data in a keyring is presented in hierarchy, so I thought I would
capture this in a GtkTreeView. Also the raw data varies, so I thought I would
present this in a 'key-value' GtkListView.

Advantages:
- Extensible. Supports varying number of attributes, and supports future
changes
  to the keyring data.
- Not too much is displayed to the user at once.

However, is it obvious to the user that the values in the list can be edited?
Should a popup dialog be used to edit, gconf-editor style?
I also have a mockup with the GTKTreeView on the left, rather than above.
I thought above was better.

See next message below for the attached glade file.

Criticism welcome.

Created attachment 27254 [details]
edit-keyring.glade attempt #1

Please, take a look at the last screenshot at:
http://developer.apple.com/documentation/Security/Conceptual/keychainServConcepts/02concepts/chapter_2_section_4.html

I like the list view there for items there, with a list below with attributes.
We'd prefer the tipical "+ Add" / "- Remove" buttons for attributes. Also the
show password checkbox and Copy to clipboard buttons are good ideas. 

Created attachment 27412 [details] [review]
Adds a basic keyring contents editing window

Here's an implementation of the keyring window. A few things about it:
 -The layout needs work (spacing, button alignment, etc).
 -It only lists items right now, not item attributes.
 -The secret field is grabbed by default right now. This probably shouldn't be.

 -An option to hide columns is needed.

Some comments:

* You need next patch, because you are freeing the value returned by
keyring_item_type_text_from_enum:
--- gnome-keyring-manager-edit.c.orig   2004-05-06 12:02:53.001201120 +0200
+++ gnome-keyring-manager-edit.c        2004-05-06 11:59:48.323276432 +0200
@@ -160,7 +160,7 @@
         break;
     }
  
-  return type_text;
+  return g_strdup (type_text);
 }
  
 static void

* When opening a keyring, the locked icon should change, because it will be
unlocked. Maybe the best way to do that is check if it's locked, and if so,
first call to unlock function (that will update the icon) and then create the
window. Also this could benefit of the new password dialog api (waiting to be
applied)

* While reading items, perhaps we need to set the busy cursor, to show that
contents are reading, and show normal cursor after last item has been read (in
update_keyring_items_worker_callback)

* I like the UI idea, but it needs some prettyfication:
   * we need a menu
   * we need hide columns option on it (by default hide id!!!)
   * I'm not sure about showing the secret in a column. (how does the apple
manager in the previous link?)
   * We should create the window inside the main file, and store a pointer to it
in a list (keyring name, window pointer and position), and if the keyring is
already opened and user try to reopen it, just present the window. If it is
closed but was opened before, open the window in the last stored position
(spatial metaphor)
   * Maybe the item type field should be replaced by an icon
   * How are we going to edit items info? (new dialog?) and attributes? (inline?)

So we need to think about this. And maybe mariano will massage the patch to
Gobjetify the window but definitively, great work!!!!

Created attachment 27424 [details]
James Bowes edit-keyring screenshot

Attached is a screenshot of James Bowes 'Edit Keyring'.

Good work James, I have a few comments, I hope they are helpful.

I agree with Fernando Herrera, it needs some prettyfication.

I think the label at the top that displays the key name should be removed. This
should be displayed in the window title. My suggestion would be 'Edit Session
Keyring', where 'Session' is the name of the keyring.
http://developer.gnome.org/projects/gup/hig/draft_hig_new/windows-primary.html#primary-window-titles


I think the labelled frames should be removed. HIG states the traditional frame
style, using borders is deprecated.
http://developer.gnome.org/projects/gup/hig/draft_hig_new/controls-frames.html

My suggestion is it should be 'New Item' rather than 'Add Item' and it should
have the 'new' icon. The 'Remove Item' button should have a trash can icon.
I don't recommend the buttons 'remove item' and 'add item' above the list.
I don't recommend having 2 add buttons and 2 remove buttons.

There should be a 12 pixel border for the window.
http://developer.gnome.org/projects/gup/hig/draft_hig_new/design-window.html#window-layout-spacing


There should be buttons to quit the window, of course this dialog is just at
early stages.

Created attachment 27430 [details] [review]
A patch against the previous, implementing craig's suggestions

This patch is applied against the previous patch. It adds Fernando's patch, as
well as attempting to implement most of Craig's UI change suggestions.

Can you post a patch from zero? g-k-m has changed quite a bit now...

I'll be away for a few days, so if someone else wants to fix this up while i'm
gone, that would be great.

Created attachment 27497 [details]
python prototype

This is a prototype for the Keyring Editor window, in python. To play with it
you need pygtk.

I am not quite in love with the lower pane; it does look very unintuitive...
What is an 'attribute'?

Created attachment 27550 [details] [review]
updated edit-keyring patch

This is a gobjectified version of the keyring editing window, fitting in with
the new code structure of g-k-m. It has the same functionality of the previous
patch.
Some of Mariano's UI design was used as well.

Created attachment 27627 [details] [review]
keyring window with menubar

This (updated) patch now adds a menubar, and checks to see if the selected
keyring is already unlocked. If not, it will prompt the user for a password to
first unlock the keyring.

Cool. This is getting into shape!
After some discussion with Mariano and Alex, we think that propabily we should
show the attributes in a Label : Value form, non-editable. Maybe we should show
only standard attributes for NETWORK_PASSWORD type (username, host, port,
comments, etc... [translated]) and add a button to edit them (edit, add,
remove). If the item contains non-standard attibutes we can show a warning and
the user could see them in the edit window. 
About the secret, I don't think it's a good idea to show it in a column. Think
about an user looking to share a password with a friend also watching the
monitor. Maybe the down panel and only showing it when the item is selected is a
better approach. 

Created attachment 27653 [details] [review]
next revision of patch

This version now has:
 -a working view menu. the user can select which columns to display.
 -the rows in the tree view now have hints (ie different colours)
 -the secret now has it's own entry field.
 -the secret is not shown by default, but may be displayed via a checkbox.
 -the secret can be copied to the clipboard by pressing the 'copy secret to
  clipboard' button.

Created attachment 27666 [details]
edit-keyring screenshot attempt #2

Fernando Herrera wrote:
"we think that propabily we should show the attributes in a Label : Value form,
non-editable"

Is this prototype what you mean by label value form? Should the value be
gtk_label instead of gtk_text_entry as shown in the screenshot?

Fernando Herrera wrote:
"and add a button to edit them (edit, add, remove)"

Is this where the edit button should be located? It prolly should have a
different icon.

Fernando Herrera wrote:
"About the secret, I don't think it's a good idea to show it in a column. Think

about an user looking to share a password with a friend also watching the
monitor. Maybe the down panel and only showing it when the item is selected is
a
better approach."

Is this what you mean by down panel? (gtk_expander). If not, I like the
expander idea. Thoughts?

Any other comments welcome.

Created attachment 27667 [details]
edit-keyring screenshot attempt #3

Forget my 'edit-keyring screenshot attempt #2' screenshot from comment 15, I
posted the wrong one! Sorry. That was an old version I was working on.

I like the way attributtes are shown in screenshot #2, and the way the password
is shown in attribute #3 (maybe using the word "secret" is better).
Your screenshot shows a "Edit" button. Maybe this edit button would bring up a
new window for editing/adding/removing attributes (and change secret type) as
well as the ACLs and the password.  And as we are going to edit in a different
window, forget the Cancel and Save buttons

Created attachment 27723 [details]
edit-keyring screenshot attempt #4

Incorporated Fernando Herrera comments.
The screenshot shows the 'Show Secret' expanded.

How does the user exit this spartial window/dialog?

Moved 'New' before 'Delete'. I think it should be this order.
I am starting to think the 'edit' button should be moved up in the same button
box, so it is 'New', 'Edit', 'Delete'.

Comments welcome.

I've committed to CVS latest Bowes' patch (from
http://torch.cs.dal.ca/~bowes/arch/ ). It follows Craig design, only the big
label for attributes should be changed for a table (like in gconf-editor). And
also the secret is presented on an GtkEntry so it can be edited direcly (the
most usual task for a normal user will be that, changing his remote password so
it's good to do that there). About moving the button... I'm not really sure.

I've replaced the single label for showing attributes with a table for nice
formatting. It's now in my arch mirror.

Please post context diff patches... 

Created attachment 27930 [details] [review]
Various updates, including change to table.

This patch adds:

whoops.

The previous patch adds:
 -tables instead of a single label.
 -the ability to change a secret.
 -saving the state of toggled columns in the editor with gconf

Created attachment 27946 [details] [review]
This one should actually apply

Same as above, but should now apply cleanly. also, readability of some of the
code is improved.

Created attachment 28050 [details] [review]
Next revision, using Mariano's suggestions

This revision adds sortable columns to the edit keyring tree view, a (very
ugly) confirmation dialog to the delete keyring action, a tooltip to the edit
button, improvements to the widget layouts, and many other ideas from Mariano.

Regarding putting the attribute values in a scrolled window, and disallowing
the window to be resized: I have had terrible luck getting this to work.
setting the window to not be resizable seems to ignore whatever dimensions
you've given it, so the window makes itself as small as possible, which becomes
a problem when displaying attributes with long values.

Created attachment 28057 [details] [review]
one that actually works

I'm terrible at making patches. this one includes _all_ of the changes

Created attachment 28086 [details] [review]
Patch against current cvs 

This is against the current cvs, and adds better alignment of the attribute
values.

Ok. I'll commit bowes' latest patch, so that we get moving on this.
We are not yet there, though!

Created attachment 28572 [details] [review]
This patch displays the note item type note in the secret field.

The 'Note' attribute type from the attribute display has been removed, since
the note is stored as the keyring item's secret.
When a note item type is selected, a multiline text editor is used to show the
secret, rather than an entry, since notes will, or could, be longer than a
password.

So, since the only item type that really has any kind of standard attribute is
the network password, what do we want to do about attributes for other types,
or the window layout?

Comment on attachment 28572 [details] [review]
This patch displays the note item type note in the secret field.

Committed plus update "Secret" / "Text" label

This is for the 0.0.x branch of g-k-m, which is no longer under active development.