make auto-reconnect cancelable/stoppable

RESOLVED FIXED in 0.2b1

Status

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: florian, Assigned: romain)

Tracking

trunk
0.2b1
Dependency tree / graph

Details

Attachments

(3 attachments, 1 obsolete attachment)

*** Original post on bio 180 at 2009-04-24 20:47:00 UTC ***

When an account is trying to reconnect, there should be a way to stop it.
Adding a "(cancel)" button presented like a link, or a cross on the line of the account where the countdown is visible would be nice.

We should also add this action to the context menu once we have it (bug 953626 (bio 179)).
Blocks: 953514, 953623
Depends on: 953626
*** Original post on bio 180 at 2009-08-02 22:28:32 UTC ***

Mass change of target milestone from 0.2a1 to 0.2 for bugs that we haven't fixed for alpha 1 but still think we are likely to fix before 0.2 final.
Target Milestone: 0.2a1 → 0.2
Posted patch Patch V1 (obsolete) — Splinter Review
*** Original post on bio 180 as attmnt 169 at 2009-08-05 20:37:00 UTC ***

Adding a link "Cancel" at the end of line of the timer.
focusable with tab, and active with enter.
Attachment #8351913 - Flags: review?(florian)
Assignee: nobody → romain
Status: NEW → ASSIGNED
Comment on attachment 8351913 [details] [diff] [review]
Patch V1

*** Original change on bio 180 attmnt 169 at 2009-08-05 21:08:51 UTC ***

>diff --git a/instantbird/base/content/instantbird/account.xml b/instantbird/base/content/instantbird/account.xml

>+    <method name="cancelReconnection">
>+      <body>
>+      <![CDATA[
>+      if (this.reconnectUpdateInterval) {
Indentation is not as in the other methods of this file. The other methods indent after opening the CDATA block.

>+        if (this.hasAttribute("reconnectPending"))
>+          this.removeAttribute("reconnectPending");
I think removeAttribute works (and just does nothing) if the attribute doesn't exist, so I don't think you need this test.

>       if (target.localName != "checkbox" &&
>+          target.getAttribute("anonid") != "cancel" &&
>           (target.localName != "button" ||
>            /^(dis)?connect$/.test(target.getAttribute("anonid"))))
I added the regexp to get the anonid attribute only once. I'm not sure if that was wise, but now that it's done, it would be more logical to just change the regexp...


>     box.appendNotification(label, "autologinStatus", null, priority, [connectNowButton]);
>   },
>+  onCancelReconnection: function am_cancelReconnection(event) {
>+    if (event.button == 0) {
>+      this.accountList.selectedItem.cancelReconnection();
>+      event.stopPropagation();
>+    }
>+  },
>   processAutoLogin: function am_processAutoLogin() {

Why did you place this method in the middle of 2 autologin-related methods?
I think I'd have put it before the connect method, though I can't seem to get an explanation of why at this place and not after the delete method...


>diff --git a/purple/purplexpcom/public/purpleIAccount.idl b/purple/purplexpcom/public/purpleIAccount.idl

>+  void cancelReconnection();
Maybe add a comment before that method. Explain that it cancels the timer that automatically reconnects accounts that were disconnected with a non fatal error.

>diff --git a/purple/purplexpcom/src/purpleAccount.cpp b/purple/purplexpcom/src/purpleAccount.cpp
>--- a/purple/purplexpcom/src/purpleAccount.cpp
>+++ b/purple/purplexpcom/src/purpleAccount.cpp
>@@ -566,16 +566,28 @@ NS_IMETHODIMP purpleAccount::Disconnect(
>   PURPLE_ENSURE_INIT(mAccount);
>   LOG(("Attempting to disconnect %s\n", mAccount->username));
> 
>   purple_account_set_enabled(mAccount, UI_ID, FALSE);
> 
>   return NS_OK;
> }
> 
>+/* void cancelReconnection(); */
>+NS_IMETHODIMP purpleAccount::CancelReconnection()
>+{
>+  if (mTimer) {
>+    mTimer->Cancel();
>+    mTimer = nsnull;
>+    /* Canceled by user action */
I don't think this comment helps. Make it more useful or remove it.

>+    mReconnectAttempts = 0;
>+  }
>+  return NS_OK;
>+}
Attachment #8351913 - Flags: review?(florian) → review-
Posted patch Patch V2Splinter Review
*** Original post on bio 180 as attmnt 170 at 2009-08-05 21:24:00 UTC ***

Fixes the coding style nits described in Comment 3.
Except the regex, it is not applicable here because the link is not a button.
Attachment #8351914 - Flags: review?(florian)
Comment on attachment 8351913 [details] [diff] [review]
Patch V1

*** Original change on bio 180 attmnt 169 at 2009-08-05 21:24:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351913 - Attachment is obsolete: true
Comment on attachment 8351914 [details] [diff] [review]
Patch V2

*** Original change on bio 180 attmnt 170 at 2009-08-06 01:11:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351914 - Flags: review?(florian) → review+
*** Original post on bio 180 at 2009-08-06 01:44:27 UTC ***

Comment on attachment 8351914 [details] [diff] [review] (bio-attmnt 170)
Patch V2

>diff --git a/instantbird/base/content/instantbird/account.xml b/instantbird/base/content/instantbird/account.xml
>--- a/instantbird/base/content/instantbird/account.xml
>+++ b/instantbird/base/content/instantbird/account.xml
>@@ -63,17 +63,23 @@
>             <xul:label flex="1" xbl:inherits="value=alias"
>                        class="accountAlias"/>
>            </xul:hbox>
>            <xul:label class="connecting" anonid="connecting" value="&account.connecting;"/>
>            <xul:label class="connected" value="&account.connected;"/>
>            <xul:label class="disconnecting" value="&account.disconnecting;"/>
>            <xul:label class="disconnected" value="&account.disconnected;"/>
>            <xul:description class="error" anonid="error"/>
>-           <xul:description class="error" anonid="reconnect"/>
>+           <xul:hbox class="error timerBox">
>+            <xul:description anonid="reconnect"/>
>+            <xul:spacer flex="1"/>
>+            <xul:label class="text-link" anonid="cancel"
>+                       value="&account.cancelReconnection.label;"
>+                       onclick="gAccountManager.onCancelReconnection(event)"/>
>+           </xul:hbox>

I've just tried the patch, and I'm not sure aligning on the right the 'cancel' button is a good idea. I guess we will need to discuss that...
*** Original post on bio 180 as attmnt 177 at 2009-08-07 23:00:00 UTC ***

Only XPCOM and js here, we need to add the "button" in a context menu to come with bug 953626 (bio 179).
Posted patch Patch V3Splinter Review
*** Original post on bio 180 as attmnt 196 at 2009-08-16 10:32:00 UTC ***

Fixing a few remaining bugs and adding an item to the context menu.
The only point I was not sure for this patch is that when an account is no longer in "pendingReconnect" state, its reconnect description is hidden using display: none in CSS, but the text itself is still there (but of course it causes no visual problem).
Attachment #8351940 - Flags: review?(florian)
Comment on attachment 8351940 [details] [diff] [review]
Patch V3

*** Original change on bio 180 attmnt 196 at 2009-08-17 01:56:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351940 - Flags: review?(florian) → review+
*** Original post on bio 180 at 2009-08-17 08:01:16 UTC ***

Additional changes were made before pushing:
In purpleAccount.idl:
-  /* Cancel the timer that automatically reconnect the account that were
+  /* Cancel the timer that automatically reconnects the account that were

and added a null check on accountList.selectedItem in disableCommandItems() to prevent this function to be called while the account manager is building (and thus have no item selected).

Pushed as https://hg.instantbird.org/instantbird/rev/7f1a472e0705
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: 0.2 → 0.2b1
You need to log in before you can comment on or make changes to this bug.