this was never implemented.  perhaps it should be desnsitised for now.

It should

Created attachment 44809 [details] [review]
disable the buttons

this should probably on the list for implementation for 2.3

patch applied for 2.1, 2.3 goal to implement it

adding patch keyword

removing PATCH keyword because  the patch doesn't implement this functionality,
it simply removes the buttons as a temporary workaround.

*** Bug 337106 has been marked as a duplicate of this bug. ***

The buttons should be removed if the feature wasn't implemented.  Insensitive buttons leave the user wondering "what option do I need to change, or plugin, to get this working?".

i second gustavo here, the buttons should not only be desensitised, but *completely removed* because the functionality is still not implemented in evolution 2.7.3.

adding srini to CC - a wonderful nosip task, don't you think? :-)
let's get this in for 2.8!

(michael's patch to desensitise the buttons has been already applied to cvs ages ago, therefore updating the patch status to committed.)

thx for a task for nosip, andre :)

welcome koolhead17! :-)

i guess that the best would be to outcomment the current buttons (if we add that functionality to evolution somewhere in the future the existing button code can be easily re-used) so they are not displayed to the user, and to add a short comment to the code like "backup isnt supported by evolution currently"...
just my two cents. :-)

Created attachment 68899 [details] [review]
hides the backup n backup all buttons

this patch hides the backup n the backup all buttons 
n also mentions the fact in the preferences that the current version
of evolution doenst support backup facility

Fixed to HEAD.

see comment #9 - the buttons should be removed instead of desensitized.

Bumping version to a stable release.

Updating bug summary.

Created attachment 285614 [details] [review]
export one key/cert pair

Thanks for the patch. I spot few minor issues, thus I thought I'll just change them and commit this, but there seem to be a little more things to be done. One things I've spot is that the saved file contains whole certificate chain, while the save from Firefox contains only that single certificate. It might be good to follow that, thus make the backups smaller.

I'm not going to name here each single change I made to your patch, I'll attach it here instead.

The things which might be good to do:
a) make sure the saved file extension will be ".p12". It makes things much
   simpler, for example when used in an attachment you can even see what is
   stored in the certificate.
b) ask for overwrite, before saving - I added a call to
   gtk_file_chooser_set_do_overwrite_confirmation(), but that doesn't work
   if user doesn't enter the .p12 extension and it is added later
c) there might be possible to save to any files, not only to local - you might
   use GFile API for this and change also (by me added) call to
   gtk_file_chooser_set_local_only()
d) there is currently not much error checking before saving the file, for
   example when user chooses a file which is not writable. This might be
   fixed for free by c)
e) there is probably no need to tell an error when user doesn't enter password
   for the new file, because telling him/her that the file is not saved,
   because he/she cancelled the password prompt is rather useless. The message
   in that password prompt would be also better descriptive, some information
   why the password is asked for would be good to have there.

Created attachment 285659 [details] [review]
updated evo patch

for evolution;

This is the promised updated version of your patch. Please continue on top of it. Also see what I've changed.

thank you for the review, I'll include your remarks into the next patch.

I guess I'll have to create a new dialog. the current solution "kind of works", but first popup file selector, then popup password (which btw does not have "repeat pwd entry") is not optimal... I just wanted to have something working first, before going into gui "design"

I'm thinking of something like:
 - file selector button
 - include/exclude cert chain option
 - pwd / repeat password
 - save / cancel

(In reply to comment #21)
> I'm thinking of something like:
>  - file selector button
>  - include/exclude cert chain option
>  - pwd / repeat password
>  - save / cancel

Works for me and will be much better than the multi-dialog backup. You can still build on top of your previous work.

Created attachment 286027 [details] [review]
BackupCert-rc2

adds a new dialog  
 - select file 
 - include cert chain
 - set password

Review of attachment 286027 [details] [review]:

I applied the patch and it works fine, thanks for it, though I have couple queries for enhancements of it.

First of all, the "Backup Certificate" dialog:
a) add mnemonics to labels, thus an Alt+mnemonic can select appropriate widget
b) the labels for widgets may end with a colon, like "File name:"
b) add there some description what is going on in this dialog. If you try to backup certificate from a FireFox, then the "Choose a Certificate Backup Password" contains some detailed information (i do not care of the 'Password quality meter', but the rest looks as a good piece of information. You might add more for the "File name" (not "Filename") as well.
c) create the checkbox in a usual way, which means:
   [x] Include certificate chain in the backup
   note of no question mark as well
d) I'm not sure whether "Passwords do not match" is really needed here
e) flip the buttons; usual button order is "Cancel | Save", though I agree there can be found places where the order is opposite.

The code-related comments (nit-picks) are below (you can click the "Review" link and see the comments in the patch, with better context, when clicking on each comment).

::: smime/gui/certificate-manager.c
@@ +563,3 @@
 
+static gboolean
+cert_backup_dialog_sensitize (GtkWidget *w, GdkEvent *event, BackupData *data)

coding style: one argument per line (multiple times in the patch)
one-letter variables are not so great, especially when searching for them in the code. Could you use more verbose names, please (multiple times in the patch)

@@ +567,3 @@
+
+	const gchar *txt1, *txt2;
+	gboolean isSensitive = TRUE;

coding style: do not use camel-case variable names, please

@@ +575,3 @@
+		isSensitive = FALSE;
+
+	if ((txt1 == NULL || strlen (txt1) == 0) && (txt2 == NULL || strlen (txt2) == 0)) {

(strlen (txt) == 0) is the same as (!*txt)

@@ +603,3 @@
+	gtk_dialog_set_default_response (GTK_DIALOG (filesel), GTK_RESPONSE_OK);
+	gtk_file_chooser_set_local_only (GTK_FILE_CHOOSER (filesel), TRUE);
+	gtk_file_chooser_set_do_overwrite_confirmation (GTK_FILE_CHOOSER (filesel), TRUE);

you should also preselect currently set file, if any.

@@ +620,3 @@
+		}
+
+		if (! g_str_has_suffix (name, ".p12")) {

coding style: extra space after '!'

@@ +622,3 @@
+		if (! g_str_has_suffix (name, ".p12")) {
+			*data->filename = g_strconcat (name, ".p12", NULL);
+			g_free (name);

when the name is changed, then I'm not asked whether I want to overwrite the file. I would try whether "confirm-overwrite" would be of any help.

@@ +633,3 @@
+			label = g_strconcat ("...", (*data->filename) + offset, NULL);
+			gtk_button_set_label (b, label);
+			g_free (label);

pity the GtkFileChooserButton cannot be used for saves. It shows only a file name, without path. Not that it's the best option (I can have the same file on multiple places), but if we'd like to mimic standard GTK's widget behaviour, then we might do that too.

@@ +647,3 @@
+
+static gint
+run_cert_backup_dialog (CertPage *cp, gchar **filename, gchar **pwd, gboolean *save_chain)

It's better to use 'password' variable name. The reason is that crash reporting tools use to highlight or otherwise process) such variables to help to avoid users exposing passwords. They cannot check for any variable names in the backtraces, thus better to make it easier to them. It also seems that gdb is not exposing content of a GString, thus using that for the 'password' parameter might partly help too.

@@ +671,3 @@
+
+	content_area = gtk_dialog_get_content_area (GTK_DIALOG (dialog));
+	g_object_set (content_area, "border-width", 6, NULL );

coding style: extra space at the end of the last argument (after the NULL here) (multiple times in the patch)

::: smime/lib/e-pkcs12.c
@@ +306,3 @@
+	SEC_PKCS12ExportContext *p12exp = NULL;
+	SEC_PKCS12SafeInfo *keySafe = NULL, *certSafe = NULL;
+	SECItem passwd;

similar as with 'pwd', use 'password' variable name instead.

@@ +358,3 @@
+	}
+
+	file = g_fopen (path, "w");

a) use "wb", just in case
b) what if the selected file fails to be created, or it is read-only on the disk? then a NULL is returned, but you do not check for this

You might get some error state checking for free when using GFile API (while you might be also able to save to not-only local folders).

(In reply to comment #24)
> 
> @@ +622,3 @@
> +        if (! g_str_has_suffix (name, ".p12")) {
> +            *data->filename = g_strconcat (name, ".p12", NULL);
> +            g_free (name);
> 
> when the name is changed, then I'm not asked whether I want to overwrite the
> file. I would try whether "confirm-overwrite" would be of any help.
> 
agreed, this was a really bad idea. Unfortunately I cannot find a way to use FileChooser signals to add the extension. “selection-changed” looked good, but when I set a new current_name the cursor is moved to the first position. I find it annoying if the cursor moves while you are typing. adding the extension in save-button-clicked did not show the confirm-dialog.

I suggest: 
  - set a default name, for example "certificate-backup.p12"
  - don't bother about correct extensions

Good idea, I like it. Do offer a default file name, please (having the file name derived from certificate's email/subject/nick/... would be also interesting).

Could you also try whether the change can be done in a "response" signal handler, when a user clicked "Save"? Just in case.

Created attachment 286408 [details] [review]
BackupCert-rc3 include remarks from Milan

this patch includes:
  - error corrections / remarks from code review
  - use GFile (please test remote files)
  - solution for extension topic based on "enter-notify-event" (requires GTK 3.10 for get_current_name() )

Thanks for the patch update. I did couple minor changes in it:
a) merge the checkbox and the "Include certificate chain in the backup" label,
   there is no need to have them split, just the opposite - I can click
   the text and it changes state of the checkbox too
b) the file extension change didn't work in both cases when pressing Enter
   after typing the name and when pressing the Save button by a mouse. I added
   another signal handler and made it work in both cases.
c) I bumped gtk+ requirement to 3.10 to have the code always working

Created commit 9c26a8d in evo master (3.13.6+) [1]

[1] https://git.gnome.org/browse/evolution/commit/?id=9c26a8d

we missed one string ...

diff --git a/smime/gui/certificate-manager.c b/smime/gui/certificate-manager.c

--- a/smime/gui/certificate-manager.c
+++ b/smime/gui/certificate-manager.c
@@ -712,7 +712,7 @@ run_cert_backup_dialog (CertPage *cp,
        data.file = file;
 
        dialog = gtk_dialog_new_with_buttons (
-               "Backup Certificate", NULL, flags,
+               _("Backup Certificate"), NULL, flags,
                _("_Cancel"), GTK_RESPONSE_CANCEL,
                _("_Save"), GTK_RESPONSE_OK,
                NULL);

Good catch, I committed it for evolution 3.13.7:
https://git.gnome.org/browse/evolution/commit/?id=1b1c8ae