Last Comment Bug 332151 - Add option to prevent spacebar from advancing to next message
: Add option to prevent spacebar from advancing to next message
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: Thunderbird 14.0
Assigned To: Rahul Abrol
:
:
Mentors:
Depends on: 746745
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-29 12:52 PST by alan
Modified: 2012-04-20 02:01 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
pref for SpaceHit function (2.86 KB, patch)
2010-02-04 22:18 PST, Rahul Abrol
bwinton: review+
Details | Diff | Splinter Review
updated pref for SpaceHit function (3.05 KB, patch)
2011-10-26 16:48 PDT, Rahul Abrol
bwinton: review+
Details | Diff | Splinter Review
proper patch (2.91 KB, patch)
2012-01-18 05:13 PST, Rahul Abrol
bwinton: review+
Details | Diff | Splinter Review
mozmill test (3.42 KB, patch)
2012-04-08 15:34 PDT, Rahul Abrol
mconley: review-
Details | Diff | Splinter Review
mozmill test (2.66 KB, patch)
2012-04-10 22:19 PDT, Rahul Abrol
mconley: review-
Details | Diff | Splinter Review
mozmill test (2.79 KB, patch)
2012-04-11 21:02 PDT, Rahul Abrol
mconley: review+
Details | Diff | Splinter Review
(AAv1) Add new default pref value to SeaMonkey too, Remove extra whitespaces [Superseded by bug 746745] (5.74 KB, patch)
2012-04-17 16:46 PDT, Serge Gautherie (:sgautherie)
iann_bugzilla: review-
Details | Diff | Splinter Review

Description alan 2006-03-29 12:52:22 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

I use the spacebar as an easy to reach alternative to the page-down key. When you reach the end of the current message, I would prefer that pushing the space bar does nothing, just as page-down does. Instead, the next message is shown. I would like a preference that disables this. 

This is easy to do, with this code change to the TB 1.5 source for mailWindowOverlay.js

line 1761 becomes  

else if (pref.getBoolPref("mail.next_message_on_spacebar")) {

instead of 

else {




Reproducible: Always
Comment 1 alan 2006-04-04 17:17:37 PDT
Given that this will probably take forever to make it into a release version (if ever), I have made a patched version of messenger.jar at http://bork.hampshire.edu/~alan/code/ which can be used with TB 1.5 (any OS).
Comment 2 Alan Robinson 2007-05-28 07:58:50 PDT
I guess nobody else wants this?
Comment 3 Rahul Abrol 2010-02-04 22:18:19 PST
Created attachment 425397 [details] [diff] [review]
pref for SpaceHit function

this also annoys me, so here's the patch, using:

pref("mail.advance_on_spacebar", true);

i haven't tested it, but as alan said, it's easy enough.
Comment 4 :aceman 2011-10-17 02:41:59 PDT
Confirming the enhancement request.
Does this patch still apply to current TB codebase (TB7-8-9)? 

You need to request review for the patch from somebody responsible for this component. See https://wiki.mozilla.org/Modules/Thunderbird .
Comment 5 Rahul Abrol 2011-10-22 18:22:38 PDT
Comment on attachment 425397 [details] [diff] [review]
pref for SpaceHit function

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

i didn't ask for review since i never tested the patch.  i still haven't though it looks like it should still apply (the line numbers are a bit off now though).
Comment 6 Blake Winton (:bwinton) (:☕️) 2011-10-26 10:33:57 PDT
Comment on attachment 425397 [details] [diff] [review]
pref for SpaceHit function

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

It's pretty simple, and it seems to work, so I think I'll say r=me, but...

Would you consider writing some mozmill tests for this?  We don't currently have any that test keyboard navigation, and I think they would be useful.

Thanks,
Blake.

::: mail/app/profile/all-thunderbird.js
@@ -182,4 +182,5 @@
> >  pref("mail.shell.checkDefaultClient", true);
> >  pref("mail.spellcheck.inline", true);
> >  pref("mail.showPreviewText", true); // enables preview text in mail alerts and folder tooltips
> >  
> > +// SpaceHit function

This bit of the diff doesn't apply anymore, so we'll probably need a new patch.
Comment 7 Rahul Abrol 2011-10-26 16:48:42 PDT
Created attachment 569843 [details] [diff] [review]
updated pref for SpaceHit function

i'll look into writing the tests.  meanwhile here's the updated patch.
Comment 8 Blake Winton (:bwinton) (:☕️) 2011-10-27 06:58:05 PDT
Comment on attachment 569843 [details] [diff] [review]
updated pref for SpaceHit function

Wow, I get the following error from the new patch:
$ hg import --no-commit https://bug332151.bugzilla.mozilla.org/attachment.cgi?i
d=569843
applying https://bug332151.bugzilla.mozilla.org/attachment.cgi?id=569843
patching file mail/base/content/mailWindowOverlay.js
patching file suite/mailnews/mailWindowOverlay.js
patching file mail/app/profile/all-thunderbird.js
abort: malformed patch a/mail/app/profile/all-thunderbird.js @@ -176,19 +176,16
@@ pref("extensions.{972ce4c6-7e08-4474-a28

So, uh, I guess I'll just mark it as r+, since it looks like we'll need to do some cleanup after importing to land this, anyways.  :)

Thanks,
Blake.
Comment 9 Ludovic Hirlimann [:Usul] 2012-01-17 05:27:10 PST
:aceman could you do the clean up ?
Comment 10 :aceman 2012-01-17 05:50:23 PST
Probably yes. But this is a recent patch. Is the submitter already away?
Comment 11 Rahul Abrol 2012-01-18 05:13:15 PST
Created attachment 589471 [details] [diff] [review]
proper patch

i haven't written any tests, but i can offer a well-formed patch.
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-01-18 08:16:07 PST
Comment on attachment 589471 [details] [diff] [review]
proper patch

Looks good to me.  I'ld still like to have some tests before we check this in, but that can be done in a separate patch.  r=me!

Rahul, would you like to try writing the tests?  There's some information to get you started at https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing#runtests and the tests themselves should be reasonably easy to write…

Thanks,
Blake.
Comment 13 Rahul Abrol 2012-01-19 01:08:51 PST
i'm much too busy to write them now, but i could probably do so before another three months passes.  if time is of the essence then i defer to someone else.
Comment 14 Rahul Abrol 2012-04-08 15:34:57 PDT
Created attachment 613201 [details] [diff] [review]
mozmill test

basic tests, added to a new "keyboard" directory.
Comment 15 Mike Conley (:mconley) 2012-04-10 08:40:59 PDT
Comment on attachment 613201 [details] [diff] [review]
mozmill test

Stealing this review.
Comment 16 Mike Conley (:mconley) 2012-04-10 09:02:37 PDT
Comment on attachment 613201 [details] [diff] [review]
mozmill test

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

Hey Rahul!  Thanks for your patch, and it's *awesome* that you're including tests.  :)

I've just found a few minor issues.  Let me know if you have any questions.

All the best,

-Mike

::: mail/test/mozmill/keyboard/test-spacehit.js
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

We should use MPL2 now (see http://www.mozilla.org/MPL/headers/)

@@ +39,5 @@
> +
> +var RELATIVE_ROOT = '../shared-modules';
> +var MODULE_REQUIRES = ['folder-display-helpers'];
> +
> +function setupModule(module) {

It looks like you're indenting 4 spaces.  For Javascript, we only indent with 2.

@@ +40,5 @@
> +var RELATIVE_ROOT = '../shared-modules';
> +var MODULE_REQUIRES = ['folder-display-helpers'];
> +
> +function setupModule(module) {
> +    let fdh = collector.getModule('folder-display-helpers');

Since fdh isn't used again, we can just use:

collector.getModule('folder-display-helpers').installInto(module);

@@ +48,5 @@
> +    make_new_sets_in_folder(folder, [{count: 3}]);
> +    be_in_folder(folder);
> +}
> +
> +function test_advance_on_spacebar(advance, shift) {

Since this isn't actually a test, but more of a test helper function, it should start with test (since the Mozmill test runner will run the function automatically).

Maybe rename this to subtest_advance_on_spacebar.

Also, this function needs documentation, and the arguments should probably start with "a", like:

aAdvance, and aShift.

@@ +50,5 @@
> +}
> +
> +function test_advance_on_spacebar(advance, shift) {
> +    // Set relevant preference
> +    Services.prefs.setBoolPref("mail.advance_on_spacebar", advance);

Instead of setting the pref here, we should put it into setupModule.

We should also store it's original state before overwriting it, and then restore the state in a teardownModule function.

@@ +61,5 @@
> +    let newmessage = mc.folderDisplay.selectedMessage;
> +    advance ? assert_not_equals(oldmessage, newmessage) : assert_equals(oldmessage, newmessage);
> +}
> +
> +function test_noadvance_on_space() {

All of these tests need documentation on what they test.  The documentation should be in the form:

/** 
 * This test does X, Y and Z because of A, B and C.
 */
function test_x_y_z() {
}
Comment 17 Rahul Abrol 2012-04-10 22:19:37 PDT
Created attachment 613876 [details] [diff] [review]
mozmill test

thanks for the feedback, mike.  i've made the requested changes and here's an updated patch.  i left the setBoolPref bit in the subtest function because it needs to change the preference halfway through the tests.  if you prefer, i can move it to setupModule and then add another function that changes the value manually.
Comment 18 Magnus Melin 2012-04-10 23:12:50 PDT
Comment on attachment 613876 [details] [diff] [review]
mozmill test

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

::: mail/test/mozmill/keyboard/test-spacehit.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Please also add a file-level doxygen comment describing the test; like

/**
 * Tests advancing to next message using space bar.
 */

For the prefs you can use Services.prefs instead if you want.
Comment 19 Mike Conley (:mconley) 2012-04-11 07:45:23 PDT
Comment on attachment 613876 [details] [diff] [review]
mozmill test

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

Looking better!  Just a few minor nits,

-Mike

::: mail/test/mozmill/keyboard/test-spacehit.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Oh, yes, I would highly prefer if you used Services.prefs over gPref.

You can get access to Services.prefs by importing it, via:

Cu.import("resource://gre/modules/Services.jsm");

And then use:

Services.prefs.getBoolPref(...) and Services.prefs.setBoolPref(...)

Good eyes, Magnus.

@@ +34,5 @@
> +  let oldmessage = select_click_row(1);
> +  wait_for_message_display_completion(mc);
> +  // Press [Shift-]Space
> +  mc.keypress(null, " ", {shiftKey: aShift});
> +  // Check that message focus changes iff $aAdvance

"$aAdvance" is a nomenclature that I don't really understand.  I'd prefer:

Check that message focus changes iff aAdvance is true.

@@ +71,5 @@
> +function test_advance_on_shiftspace() {
> +  subtest_advance_on_spacebar(true, true);
> +}
> +
> +function teardownModule(module) {

This function should go beneath the setupModule function.
Comment 20 Rahul Abrol 2012-04-11 21:02:26 PDT
Created attachment 614269 [details] [diff] [review]
mozmill test

version 3.
Comment 21 Mike Conley (:mconley) 2012-04-16 12:45:46 PDT
Comment on attachment 614269 [details] [diff] [review]
mozmill test

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

I'm happy with this.  Thanks, Rahul!

-Mike
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-04-16 14:35:57 PDT
http://hg.mozilla.org/comm-central/rev/f996fd932384
http://hg.mozilla.org/comm-central/rev/da1d6062b37e

Thanks for the patch! In the future, to make life easier for those checking in your patches for you, please follow the directions below for proper patch formatting. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-04-16 17:03:33 PDT
Follow-up patch so that the new test is actually run...
http://hg.mozilla.org/comm-central/rev/c603ee7bcda3
Comment 24 Mike Conley (:mconley) 2012-04-16 17:04:59 PDT
(In reply to Ryan VanderMeulen from comment #23)
> Follow-up patch so that the new test is actually run...
> http://hg.mozilla.org/comm-central/rev/c603ee7bcda3

Argh, good catch Ryan.  Thanks.
Comment 25 Serge Gautherie (:sgautherie) 2012-04-16 17:05:28 PDT
(In reply to Ryan VanderMeulen from comment #22)
> http://hg.mozilla.org/comm-central/rev/f996fd932384

Thanks for keeping SeaMonkey sync'ed!
Yet, "mail.advance_on_spacebar" pref should live in mailnews/ not in mail/, so SeaMonkey gets it too...
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-04-16 17:16:21 PDT
(In reply to Mike Conley (:mconley) from comment #24)
> (In reply to Ryan VanderMeulen from comment #23)
> > Follow-up patch so that the new test is actually run...
> > http://hg.mozilla.org/comm-central/rev/c603ee7bcda3
> 
> Argh, good catch Ryan.  Thanks.

I filed bug 746014 for some other directories that were missing from the manifest.
Comment 27 Jens Hatlak (:InvisibleSmiley) 2012-04-17 01:43:08 PDT
(In reply to Serge Gautherie (:sgautherie) from comment #25)
> (In reply to Ryan VanderMeulen from comment #22)
> > http://hg.mozilla.org/comm-central/rev/f996fd932384
> 
> Thanks for keeping SeaMonkey sync'ed!
> Yet, "mail.advance_on_spacebar" pref should live in mailnews/ not in mail/,
> so SeaMonkey gets it too...

Right, either that or a matching entry in browser-prefs.js. Follow-up or here?
Comment 28 Serge Gautherie (:sgautherie) 2012-04-17 16:46:37 PDT
Created attachment 615950 [details] [diff] [review]
(AAv1) Add new default pref value to SeaMonkey too, Remove extra whitespaces
[Superseded by bug 746745]
Comment 29 Ian Neal 2012-04-18 17:01:19 PDT
Comment on attachment 615950 [details] [diff] [review]
(AAv1) Add new default pref value to SeaMonkey too, Remove extra whitespaces
[Superseded by bug 746745]

Most shared mailnews prefs go into mailnews/mailnews.js so I would prefer it there.
Comment 30 Serge Gautherie (:sgautherie) 2012-04-18 17:59:05 PDT
(In reply to Ian Neal from comment #29)
> Most shared mailnews prefs go into mailnews/mailnews.js so I would prefer it
> there.

I tried it per comment 27, as this doesn't (currently) depend on shared code.
But if you prefer it per comment 25, then let's reopen this bug.
Comment 31 Magnus Melin 2012-04-18 22:35:15 PDT
Closing as fixed, please do it in bug 746745 instead.

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