332151 - Add option to prevent spacebar from advancing to next message

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[alan]

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]

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]

I guess nobody else wants this?

Comment 3

[Rahul Abrol]

Attachment: 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]

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]

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) (:☕️)]

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]

Attachment: updated pref for SpaceHit function

i'll look into writing the tests.  meanwhile here's the updated patch.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

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]]

:aceman could you do the clean up ?

Comment 10

[:aceman]

Probably yes. But this is a recent patch. Is the submitter already away?

Comment 11

[Rahul Abrol]

Attachment: proper patch

i haven't written any tests, but i can offer a well-formed patch.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

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]

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]

Attachment: mozmill test

basic tests, added to a new "keyboard" directory.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 613201 [details] [diff] [review]
mozmill test

Stealing this review.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

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]

Attachment: 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 [:mkmelin]]

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) (:⚙️)]

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]

Attachment: mozmill test

version 3.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

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]]

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]]

Follow-up patch so that the new test is actually run...
http://hg.mozilla.org/comm-central/rev/c603ee7bcda3

Comment 24

[Mike Conley (:mconley) (:⚙️)]

(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)]

(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]]

(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)]

(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)]

Attachment: (AAv1) Add new default pref value to SeaMonkey too, Remove extra whitespaces [Superseded by bug 746745]

Comment 29

[Ian Neal]

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)]

(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 [:mkmelin]]

Closing as fixed, please do it in bug 746745 instead.