Last Comment Bug 749690 - Add a findbar and support the zoom feature in the chat tab
: Add a findbar and support the zoom feature in the chat tab
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 15.0
Assigned To: Florian Quèze [:florian] [:flo]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-27 10:33 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-05-07 00:55 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch (3.25 KB, patch)
2012-04-27 10:33 PDT, Florian Quèze [:florian] [:flo]
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch for checkin (3.27 KB, patch)
2012-05-01 07:54 PDT, Florian Quèze [:florian] [:flo]
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] 2012-04-27 10:33:50 PDT
Created attachment 619094 [details] [diff] [review]
Patch

I was mostly interested in adding the findbar as we will want to use it for a follow up to bug 743235, but while looking at how the find commands work in other Thunderbird tabs, I noticed I could get the zoom working almost for free, and as we will want it too anyway, I put both in the same bug and patch.
Comment 1 Blake Winton (:bwinton) (:☕️) 2012-04-30 11:09:33 PDT
Comment on attachment 619094 [details] [diff] [review]
Patch

I just noticed something which you might want to fix in this patch. Ctrl-K doesn't focus the global search bar, even though the empty text says it does.

>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -124,20 +124,93 @@ var chatTabType = {
>+  supportsCommand: function supportsCommand(aCommand, aTab) {

nit: I think we usually name these "ct_supportsCommand", just so that we can more easily see where they're coming from.

>+  doCommand: function isCommandEnabled(aCommand, aTab) {

And this function really shouldn't be named "isCommandEnabled".  ;)

>+  getBrowser: function getBrowser(aTab) {
>+    let panel = document.getElementById("conversationsDeck").selectedPanel;
>+    if (panel == document.getElementById("logDisplay")) {
>+      if (document.getElementById("logDisplayDeck").selectedPanel ==
>+          document.getElementById("logDisplayBrowserBox"))
>+        return document.getElementById("conv-log-browser");
>+    }
>+    else if (panel && panel.localName == "imconversation")
>+      return panel.browser;

As mentioned in a previous review, there should be {}s around this else clause (and the one a little further down).

Other than that I'm pretty happy.  r=me, and ui-r=me.

Later,
Blake.
Comment 2 Florian Quèze [:florian] [:flo] 2012-05-01 07:03:23 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #1)
> Comment on attachment 619094 [details] [diff] [review]
> Patch
> 
> I just noticed something which you might want to fix in this patch. Ctrl-K
> doesn't focus the global search bar, even though the empty text says it does.

I have a patch ready for check-in addressing this specific issue in bug 749552.
Comment 3 Florian Quèze [:florian] [:flo] 2012-05-01 07:54:52 PDT
Created attachment 619928 [details] [diff] [review]
Patch for checkin

Addresses the nits from comment 1.
Comment 4 Florian Quèze [:florian] [:flo] 2012-05-02 03:29:08 PDT
http://hg.mozilla.org/comm-central/rev/60c7b4af09cb
Comment 5 Mark Banner (:standard8) 2012-05-07 00:55:34 PDT
Checked into aurora:

https://hg.mozilla.org/releases/comm-aurora/rev/cd0ce382db04

Note You need to log in before you can comment on or make changes to this bug.