Closed Bug 953616 Opened 7 years ago Closed 7 years ago

Change default actions on the "Account manager dialog"

Categories

(Instantbird :: Account manager, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: romain)

References

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 169 at 2009-03-14 23:58:00 UTC ***

Idea: set the keyboard default (= action on pressing 'enter') and the double-click default action to connect/disconnect account.
This behavious seems to be more 'natural' to me.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Other → All
Hardware: x86 → All
Blocks: 953623
Target Milestone: --- → 0.2a1
*** Original post on bio 169 at 2009-08-02 22:28:26 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
Attached patch Patch V1 (obsolete) — Splinter Review
*** Original post on bio 169 as attmnt 163 at 2009-08-04 20:42:00 UTC ***

I added a method which proceed connect() or disconnect() if the button is not disabled and the state if either connected or disconnected (it does nothing if the state is connecting or disconnecting).

This method is called when the user dbleclick on the account (does not work if dbleclick is on the checkbox or a button), and the same method is called if DOM_VK_ENTER is pressed (the event handler is in the richlistbox this time).
Attachment #8351907 - Flags: review?(florian)
Assignee: nobody → romain
Status: NEW → ASSIGNED
*** Original post on bio 169 at 2009-08-04 21:42:30 UTC ***

Comment on attachment 8351907 [details] [diff] [review] (bio-attmnt 163)
Patch V1

>diff --git a/instantbird/base/content/instantbird/account.xml b/instantbird/base/content/instantbird/account.xml
>+     <method name="proceedDefaultAction">
>+      <body>
>+      <![CDATA[
>+        // Get the state of the visible button
>+        let buttonElt = this._account.disconnected ? "connect" : "disconnect";
>+        if (document.getAnonymousElementByAttribute(this, "anonid", buttonElt)
>+                    .disabled)
Would something like this just work:
        document.getAnonymousElementByAttribute(this, "anonid", buttonElt)
                .click();

I'm not completely sure, but I think the click() method exists, and it is likely to do nothing if the button is disabled. Could you test this?
Attached patch Patch V2 (obsolete) — Splinter Review
*** Original post on bio 169 as attmnt 164 at 2009-08-04 21:58:00 UTC ***

click() works.
It drops the amount of lines by 5 in this new patch.
Attachment #8351908 - Flags: review?(florian)
Comment on attachment 8351907 [details] [diff] [review]
Patch V1

*** Original change on bio 169 attmnt 163 at 2009-08-04 21:58:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351907 - Attachment is obsolete: true
Attachment #8351907 - Flags: review?(florian)
Comment on attachment 8351908 [details] [diff] [review]
Patch V2

*** Original change on bio 169 attmnt 164 at 2009-08-04 22:31:53 UTC ***

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

>+     <method name="proceedDefaultAction">
>+      <body>
>+      <![CDATA[
>+        // Get the state of the visible button
This comment is not really understandable.
What about '// Determine which action is possible'? Or maybe just remove the comment?

>+        let buttonElt = this._account.disconnected ? "connect" : "disconnect";
>+        document.getAnonymousElementByAttribute(this, "anonid", buttonElt).click()
As we have just changed the variable name in bug 953650 (bio 204), I think we should call the variable 'action' here too.

>+      if (localName != "button" && localName != "checkbox")
I'm not really happy with this test (I'd prefer instanceof checks) but given that there was already a similar test in the code around, I guess I'll just accept it for now.

r=me with those details addressed. I can do this before pushing the patch if you don't want to attach yet another patch for so minor details.
Attachment #8351908 - Flags: review?(florian) → review+
*** Original post on bio 169 as attmnt 166 at 2009-08-04 23:00:00 UTC ***

During testing I found another issue: when pressing enter while the button of the default action is focused, nothing happens. The attached modified patch fixes this issue (and the details mentioned in the previous review comments).
Comment on attachment 8351908 [details] [diff] [review]
Patch V2

*** Original change on bio 169 attmnt 164 at 2009-08-04 23:00:57 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351908 - Attachment is obsolete: true
*** Original post on bio 169 at 2009-08-04 23:13:38 UTC ***

attachment 8351910 [details] [diff] [review] (bio-attmnt 166) pushed:
https://hg.instantbird.org/instantbird/rev/ef152e0d2fe6
Status: ASSIGNED → RESOLVED
Closed: 7 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.