340560 - Integrate the replication progress dialog into the LDAP directory properties window

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

The replication progress dialog doesn't really need to be a separate window, there is some nice real estate available in the directory properites window just underneath the download button. So combining them should reduce codesize.

Additionally this will let us fix some bugs with it like trying to kick off a replication process twice by accidently double clicking on the button etc.

I've a patch coming up.

Comment 1

[Mark Banner (:standard8)]

Attachment: Patch v1

The patch. Implementation for both sm & tb.

Comment 2

[[e-mail]]

Comment on attachment 224599 [details] [diff] [review]
Patch v1

>+  document.getElementById("download").label =
>+    gReplicationBundle.getString("downloadButton");
I'm not convinced that this can be accessibly localised, but feel free to double-check with KaiRo.

>+      gProgressMeter.setAttribute("mode","undetermined");
Nit: space after comma (and also in EndDownload).

>+      gDownloadInProgress = true;
This is confusing because clicking the button changes it into a cancel button without changing this flag, so it doesn't actually cancel yet. Maybe you should change the button here too.

>+      EndDownload(aStatus);
This confused me until I realised that the C++ is actually abusing nsresults.

>+  gProgressText.value =
>+    gReplicationBundle.getString(aStatus ? "replicationSucceeded" :
>+                                 (gReplicationCancelled ? "replicationCancelled" :
>+                                  "replicationFailed"));
Nit: don't need ()s as ?: is right associative.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> (From update of attachment 224599 [details] [diff] [review] [edit])
> >+  document.getElementById("download").label =
> >+    gReplicationBundle.getString("downloadButton");
> I'm not convinced that this can be accessibly localised, but feel free to
> double-check with KaiRo.

Well the two labels that could be used both contain "D" which I've set permantely. I can do Alt-D for both of them (on Linux) and that works fine.

Comment 4

[Mark Banner (:standard8)]

Attachment: Patch v2

Addressed Neil's comments and carried forward his sr.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Addressed Neil's comments and carried forward his sr.

Sorry, I meant r.

Comment 6

[Robert Kaiser]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 224599 [details] [diff] [review] [edit] [edit])
> > >+  document.getElementById("download").label =
> > >+    gReplicationBundle.getString("downloadButton");
> > I'm not convinced that this can be accessibly localised, but feel free to
> > double-check with KaiRo.
> 
> Well the two labels that could be used both contain "D" which I've set
> permantely. I can do Alt-D for both of them (on Linux) and that works fine.

Please don't do this. How do you know that all languages contain the same character in both strings and have no conflict of that character with other access keys?

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 225137 [details] [diff] [review]
Patch v2

Cancelling review due to KaiRo's comments.

Comment 8

[Mark Banner (:standard8)]

Attachment: Patch v3

New patch that uses two buttons in the xul - it appears that the access key can't be changed dynamically from js, and therefore we need two buttons. Requesting review again due to the changes required.

Comment 9

[Mark Banner (:standard8)]

Attachment: Patch v4 (checked in to trunk)

Based on the patch this uses two access keys for the button to avoid localisation issues.

Comment 10

[Mark Banner (:standard8)]

Attachment: Remove old file references from the jar.mn files (checked in to trunk)

I forgot to remove the references to the old files from the jar.mn files.... thankfully I haven't removed the old ones yet.

Comment 11

[Mark Banner (:standard8)]

Attachment: Combined patch for branch (checked in)

This patch combines the previous two for branch checkin (no other changes). Requesting branch approval - this would be a nice little enhancement to the ldap setup for SeaMonkey & Thunderbird. Should be low risk. It has some l10n impact so I'd like to get it in before 15th June if possible - though I don't know if that just applies to firefox or not.

Comment 12

[Mark Banner (:standard8)]

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