Last Comment Bug 708550 - Cannot copy version string from "About Thunderbird" dialogue window (regression)
: Cannot copy version string from "About Thunderbird" dialogue window (regression)
Status: VERIFIED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 22.0
Assigned To: sakshi
:
Mentors: Thomas D. (currently busy elsewhere; needinfo?me)
rsx11m
Depends on:
Blocks: 572769
  Show dependency treegraph
 
Reported: 2011-12-08 01:33 PST by Thomas D. (currently busy elsewhere; needinfo?me)
Modified: 2014-07-27 05:53 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP (2.50 KB, patch)
2013-02-19 19:48 PST, sakshi
no flags Details | Diff | Splinter Review
WIP (2.50 KB, patch)
2013-02-19 19:50 PST, sakshi
bugzilla2007: review-
Details | Diff | Splinter Review
WIP (2.51 KB, patch)
2013-02-20 03:12 PST, sakshi
bugzilla2007: review+
Details | Diff | Splinter Review
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF) (2.51 KB, patch)
2013-02-25 05:31 PST, sakshi
mconley: review+
bugzilla2007: feedback+
Details | Diff | Splinter Review

Description Thomas D. (currently busy elsewhere; needinfo?me) 2011-12-08 01:33:13 PST
Nitfix:

STR

1) Help > About TB
2) Try to copy version string (for bug reports, support requests etc.), e.g. for Daily: "11.0a1 (2011-12-08)"

Actual
- cannot copy version string (not even with mouse)

Expected
- at least allow mouse-selection of version string for copying
- better: make version string keyboard-accessible (focus and selection)
- this was working in previous versions of TB (regression)
- this is working in FF 8 and FF Nightly (so it's us who broke it, or didn't fix it)

This should be really trivial to fix, perhaps just display the version string in an invisible text field, so that standard methods for keyboard and mouse access are available (at least we should not be worse than FF).

FTR: This is NOT a duplicate of Bug 572769 (where this was still working), and going to Help > Troubleshooting (about:support) instead is NOT a solution for this bug (hell no, I don't want to search for a simple version string between dozens of entries, and then copy an unreadable User Agent monster string like "Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111205 Thunderbird/11.0a1").
Comment 1 Thomas D. (currently busy elsewhere; needinfo?me) 2011-12-08 02:10:33 PST
While we're here, we should also fix focus behaviour and indication:

Currently, initial focus is always on "Apply downloaded update" button, or on the "TB is up to date" label, so first TAB will always take you to mozilla.org link.
However, we are inconsistent about the focus ring on the button: Initially, no focus ring (but button has focus), but: TAB, then Shift+TAB -> Button has focus AND focus ring. We should decide if the button should have a focus ring (or not) when focused (probably yes), and then make the behaviour consistent for initial and subsequent manual focus.

So starting out from current initial focus which is always on "Apply..." button or "...up to date" label, *Shift+TAB* from there should always focus and select the version string right above the button location (proposed solution for this bug). Thus, we are not changing the forward-TAB sequence, AND we are helpful and well-behaved about focusing and selecting the version string with first Shift+TAB for helpful folks who'd like to copy that. Simple. Nothing extraordinary here.
Comment 2 rsx11m 2011-12-08 07:42:42 PST
Is the copy functionality supposed to work for the pre-release builds only?
I can't mark the "8.0" string in the current release already... (Windows 7)
Comment 3 David E. Ross 2011-12-10 10:35:35 PST
Users should be able to select and copy all text within the About Mozilla Thunderbird popup.  

While composing bug #709489, I tried to copy the text for the links at the bottom of the popup; this was not possible.  Instead, I had to contrive small windows for both Thunderbird and SeaMonkey so that I could see the text in the popup while manually typing the text into the Bugzilla page in SeaMonkey.
Comment 4 Wayne Mery (:wsmwk, NI for questions) 2012-11-03 06:41:19 PDT
FWIW, works in firefox
Comment 5 Wayne Mery (:wsmwk, NI for questions) 2012-11-04 03:06:06 PST
(In reply to Wayne Mery (:wsmwk) from comment #4)
> FWIW, works in firefox

code can be copied from bug 598169
Comment 6 sakshi 2013-02-19 08:25:12 PST
Hello,

I would like to work on this bug.
Comment 7 sakshi 2013-02-19 19:48:01 PST
Created attachment 715851 [details] [diff] [review]
WIP
Comment 8 sakshi 2013-02-19 19:50:14 PST
Created attachment 715854 [details] [diff] [review]
WIP
Comment 9 Thomas D. (currently busy elsewhere; needinfo?me) 2013-02-20 00:37:26 PST
Comment on attachment 715854 [details] [diff] [review]
WIP

Review of attachment 715854 [details] [diff] [review]:
-----------------------------------------------------------------

For a start, we want to do the same thing here as FF, so this looks good at first sight (I'll check the details later). However, there's one line which breaks the patch.

::: mail/base/content/aboutDialog.xul
@@ +40,5 @@
>    <vbox id="aboutDialogContainer">
>      <hbox id="clientBox">
>        <vbox id="leftBox" flex="1"/>
>        <vbox id="rightBox" flex="1">
> +#expand <label id="version"> 17.0.2</label>

Pls don't hard-code the version number, you need to keep the variable, in analogy to the FF patch.
Comment 10 sakshi 2013-02-20 00:41:38 PST
Yes, I tried that. But when i tried __MOZ_APP_VERSION__ it was directly printing this on the ' About thunderbird' dialog
Comment 11 Richard Marti (:Paenglab) 2013-02-20 01:06:50 PST
You have patched the files directly in omni.ja to test?
__MOZ_APP_VERSION__ will be exchanged during build (compiling) with the correct string.
Comment 12 sakshi 2013-02-20 03:12:43 PST
Created attachment 715951 [details] [diff] [review]
WIP
Comment 13 rsx11m 2013-02-20 05:57:16 PST
(In reply to Richard Marti [:Paenglab] from comment #11)
> __MOZ_APP_VERSION__ will be exchanged during build (compiling) with the
> correct string.

Yes, everything starting with a "#something" is a pre-processor directive and will thus only be effective when you create your own build. Thus, you'll see the actual version number hardcoded in the omni.ja's XUL file, which is different from the given source found in the repository.

Also, flagging the patch for "review?" is good, but you have to pick a reviewer (module owner or peer) to assign the review.
Comment 14 Thomas D. (currently busy elsewhere; needinfo?me) 2013-02-20 10:21:49 PST
(In reply to rsx11m from comment #13)
> Also, flagging the patch for "review?" is good, but you have to pick a
> reviewer (module owner or peer) to assign the review.

o In the spirit of the new New Release and Governance Model (1) which requires more collaboration and community-driven involvment,
o encouraged by respective statements in a recent status meeting (2), and
o emboldened by the fast & successful cooperative review experience in bug 581470 (also involving me and Sakshi)
I hereby submit that I'd like to snatch the review for this simple patch! :)

Which potentially spares some valuable time and energy of main reviewers for higher purposes. However, I'll gladly request an additional review from them when I'm done.

(1) https://wiki.mozilla.org/Thunderbird/Proposal:_New_Release_and_Governance_Model
(2) https://wiki.mozilla.org/Thunderbird/StatusMeetings/2013-02-05#Question_Time
Comment 15 rsx11m 2013-02-20 12:24:16 PST
Sure, no objections from this end. :-)

I was mostly prompted to that comment given that there is no name associated with Sakashi's review request, thus wanted to point out that it might get lost without naming a specific reviewer.
Comment 16 Thomas D. (currently busy elsewhere; needinfo?me) 2013-02-22 09:07:35 PST
Comment on attachment 715951 [details] [diff] [review]
WIP

Review of attachment 715951 [details] [diff] [review]:
-----------------------------------------------------------------

Sakshi, thanks for the quick patch :)

As announced and explained in my comment 14, I'll snatch the review for this trivial patch which is identical to FF patch of attachment 539592 [details] [diff] [review] :)

r=me with the following nit fixed:

::: mail/base/content/aboutDialog.js
@@ +36,5 @@
>    let version = Services.appinfo.version;
>    if (/a\d+(pre)?$/.test(version)) {
>      let buildID = Services.appinfo.appBuildID;
>      let buildDate = buildID.slice(0,4) + "-" + buildID.slice(4,6) + "-" + buildID.slice(6,8);
> +    document.getElementById("version").textContent += " (" + buildDate + ")";

Looking at the source (1) in mxr, this line has already been fixed (perhaps recently - pls ensure you're patching against the most recent files), so you can remove aboutDialog.js entirely from your patch.

(1) http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.js#40
Comment 17 rsx11m 2013-02-22 10:43:01 PST
(In reply to Thomas D. from comment #16)
> Looking at the source (1) in mxr, this line has already been fixed (perhaps
> recently - pls ensure you're patching against the most recent files), so you
> can remove aboutDialog.js entirely from your patch.

Eh, yes - it was indeed fixed, but in mozilla-central, not in comm-central...  ;-)

> (1)
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.js#40

vs.

> http://mxr.mozilla.org/comm-central/source/mail/base/content/aboutDialog.js#40

However, the patch doesn't apply for me against comm-central even though it should:

> % patch -p1 < about.patch 
> patching file mail/base/content/aboutDialog.css
> patching file mail/base/content/aboutDialog.js
> patching file mail/base/content/aboutDialog.xul
> Hunk #1 FAILED at 36.
> 1 out of 1 hunk FAILED -- saving rejects to file mail/base/content/aboutDialog.xul.rej

The reason being that it has two trailing spaces after the [+/-]#expand lines = removing those makes the patch apply cleanly for me.
Comment 18 Thomas D. (currently busy elsewhere; needinfo?me) 2013-02-22 12:01:52 PST
(In reply to rsx11m from comment #17)
> (In reply to Thomas D. from comment #16)
> > Looking at the source (1) in mxr, this line has already been fixed (perhaps
> > recently - pls ensure you're patching against the most recent files), so you
> > can remove aboutDialog.js entirely from your patch.
> 
> Eh, yes - it was indeed fixed, but in mozilla-central, not in
> comm-central...  ;-)

Thanks for the correction!

It is indeed fixed in mozilla-central...
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog.js#40

...AND in comm-central's browser component (where I got confused because it's also inside comm-central, but not on the mail root):

> > (1)
> > http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/aboutDialog.js#40
> vs.

...but not yet in TB:
> > http://mxr.mozilla.org/comm-central/source/mail/base/content/aboutDialog.js#40

So aboutDialog.js needs to stay in the patch as is, but those trailing spaces in the #replace lines of aboutDialog.xul need to be removed. Thanks.
Comment 19 rsx11m 2013-02-22 12:46:58 PST
(In reply to Thomas D. from comment #18)
> ...AND in comm-central's browser component (where I got confused because
> it's also inside comm-central, but not on the mail root):

No, there is no browser component in comm-central itself. Everything within the mozilla/ directory is the complete mozilla-central tree, thus including Firefox's browser/ sources even though they are not used. The mozilla/ directory is created when running "python client.py checkout" after you've checked out the comm-central repository, to get the dependent content.

Note that http://hg.mozilla.org/comm-central doesn't show the mozilla/ directory when browsing the current changeset, so that's something MXR seems to do on its own when searching it.
Comment 20 Magnus Melin 2013-02-23 12:32:08 PST
(In reply to Thomas D. from comment #14)
> (In reply to rsx11m from comment #13)
> o encouraged by respective statements in a recent status meeting (2), and
> (2)
> https://wiki.mozilla.org/Thunderbird/StatusMeetings/2013-02-05#Question_Time

To clarify, that "anyone" in "can anyone steal reviews" was really meant "anyone that is a submodule peer of another module", unless i'm mistaken (though i didn't make if for the meeting). We certainly don't have a policy that *anyone* can stamp review on something just because they think it's trivial.

That's not to say it can't be helpful for reviewers if someone have time to check patches work and are as expected in mozilla, especially for people not yet familiar with the procedures. However, please use the feedback flag for that instead, otherwise it can be hard for someone new to know what's really expected. It also easily get wrong review flags when someone checks the patch in.
Comment 21 sakshi 2013-02-25 05:31:52 PST
Created attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)
Comment 22 rsx11m 2013-02-25 06:44:58 PST
Comment on attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

Sakashi, I'm changing your review request to one for feedback per comment #20.
Meaning, this will still require a review by a module peer.
Comment 23 sakshi 2013-02-25 09:05:09 PST
Okay. So who will be reviewing the patch?
Comment 24 Richard Marti (:Paenglab) 2013-02-25 09:10:20 PST
Mike Conley would be a good candidate.
Comment 25 sakshi 2013-02-25 09:13:08 PST
Okay. So I should set him to review the patch. His email-address?
Comment 26 rsx11m 2013-02-25 09:13:38 PST
Mike (type mconley to autocomplete his e-mail) was the primary reviewer already on your previous patch. Wait for Thomas' feedback and then you can request review from him (and there are more in case he won't have time to look into it).
Comment 27 rsx11m 2013-02-25 09:15:11 PST
(or cancel the feedback and go directly for the review, your choice.)
Comment 28 sakshi 2013-02-25 09:18:22 PST
That's okay. I can wait for the feedback from Thomas and then ask for review.
Comment 29 Thomas D. (currently busy elsewhere; needinfo?me) 2013-02-25 09:22:41 PST
Comment on attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

Review of attachment 717839 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks correct now.
Comment 30 Thomas D. (currently busy elsewhere; needinfo?me) 2013-02-25 10:21:26 PST
Comment on attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

Mike, another 5-line patch by new contributor Sakshi awaiting your review in the footsteps of bug 581470, where the cooperative review process around your rapid technical review enabled that patch to land within only 14 days. Encouraged by that experience, here's Sakshi's next patch only 1 week after that. And she's eager for more as soon as this gets cleared by you :)

To ease your review, we have pre-reviewed the patch and it applies cleanly per  rsx11m's comment 17. This is the twin patch of attachment 539592 [details] [diff] [review] from FF bug 598169, where the "About Firefox" dialogue has been tweaked in exactly the same way.
Comment 31 Mike Conley (:mconley) - (Needinfo me!) 2013-02-25 10:32:19 PST
Comment on attachment 717839 [details] [diff] [review]
Patch: Enable selection of version string in "About Thunderbird" dialogue for copying (as in FF)

Review of attachment 717839 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, this looks good to go! Thanks sakshi!
Comment 32 rsx11m 2013-02-25 10:36:25 PST
Sakshi, you have all approvals needed now for your patch and can add "checkin-needed" to the keywords at the top of this bug. Someone will check it into the code with the next round.
Comment 33 Thomas D. (currently busy elsewhere; needinfo?me) 2013-02-25 10:43:28 PST
(In reply to rsx11m from comment #32)
> Sakshi, you have all approvals needed now for your patch and can add
> "checkin-needed" to the keywords at the top of this bug. Someone will check
> it into the code with the next round.

It needs more rights than Sakshi currently has to change keywords ;)
Comment 34 rsx11m 2013-02-25 10:53:23 PST
Thanks Thomas. In theory, changing the basic fields should be permitted to her given that I made her the assignee (at least this is how it used to work in bugzilla in the past, unless that got removed with some update).
Comment 35 Wayne Mery (:wsmwk, NI for questions) 2013-02-25 11:10:17 PST
(In reply to Thomas D. from comment #33)
> It needs more rights than Sakshi currently has to change keywords ;)

we can fix that if Sakshi agrees to fix 50 more bugs!

:) Just kidding. 

I don't know if privs are needed, but I have just granted them. Sakshi can now change field and confirm/close bugs.

In general, there are problems of this type for newbies, such as bug 802269
Comment 36 Ryan VanderMeulen [:RyanVM] 2013-02-25 11:51:56 PST
https://hg.mozilla.org/comm-central/rev/6282d8307215
Comment 37 rsx11m 2013-02-25 13:09:55 PST
Verified fixed in tinderbox build linux64/1361821896, (X11; Linux x86_64; rv:22.0) Thunderbird/22.0a1; I can mark and copy "22.0a1 (2013-02-25)" now to the clipboard, and it pastes fine into a text window.

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