Add option to prevent spacebar from advancing to next message

RESOLVED FIXED in Thunderbird 14.0

Status

Thunderbird
Mail Window Front End
--
enhancement
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: alan, Assigned: Rahul Abrol)

Tracking

Trunk
Thunderbird 14.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
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).
QA Contact: front-end

Comment 2

10 years ago
I guess nobody else wants this?

Updated

9 years ago
Assignee: mscott → nobody
(Assignee)

Comment 3

8 years ago
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

6 years ago
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 .
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

6 years ago
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).
Attachment #425397 - Flags: review?(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.
Attachment #425397 - Flags: review?(bwinton) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 569843 [details] [diff] [review]
updated pref for SpaceHit function

i'll look into writing the tests.  meanwhile here's the updated patch.
Attachment #425397 - Attachment is obsolete: true
Attachment #569843 - Flags: review?(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.
Attachment #569843 - Flags: review?(bwinton) → review+
:aceman could you do the clean up ?

Comment 10

6 years ago
Probably yes. But this is a recent patch. Is the submitter already away?
(Assignee)

Comment 11

6 years ago
Created attachment 589471 [details] [diff] [review]
proper patch

i haven't written any tests, but i can offer a well-formed patch.
Attachment #569843 - Attachment is obsolete: true
Attachment #589471 - Flags: review?
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.
Attachment #589471 - Flags: review? → review+
(Assignee)

Comment 13

6 years ago
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.

Updated

6 years ago
Assignee: nobody → solushex
Status: NEW → ASSIGNED
Version: unspecified → Trunk
(Assignee)

Comment 14

5 years ago
Created attachment 613201 [details] [diff] [review]
mozmill test

basic tests, added to a new "keyboard" directory.
Attachment #613201 - Flags: review?(bwinton)
Comment on attachment 613201 [details] [diff] [review]
mozmill test

Stealing this review.
Attachment #613201 - Flags: review?(bwinton) → review?(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() {
}
Attachment #613201 - Flags: review?(mconley) → review-
(Assignee)

Comment 17

5 years ago
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.
Attachment #613201 - Attachment is obsolete: true
Attachment #613876 - Flags: review?(mconley)

Comment 18

5 years ago
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 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.
Attachment #613876 - Flags: review?(mconley) → review-
(Assignee)

Comment 20

5 years ago
Created attachment 614269 [details] [diff] [review]
mozmill test

version 3.
Attachment #613876 - Attachment is obsolete: true
Attachment #614269 - Flags: review?(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
Attachment #614269 - Flags: review?(mconley) → review+

Updated

5 years ago
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 14.0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Follow-up patch so that the new test is actually run...
http://hg.mozilla.org/comm-central/rev/c603ee7bcda3
(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.
(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...
(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.
(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?
Created attachment 615950 [details] [diff] [review]
(AAv1) Add new default pref value to SeaMonkey too, Remove extra whitespaces
[Superseded by bug 746745]
Attachment #615950 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Blocks: 746745

Comment 29

5 years ago
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.
Attachment #615950 - Flags: review?(iann_bugzilla) → review-
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 31

5 years ago
Closing as fixed, please do it in bug 746745 instead.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
No longer blocks: 746745
Depends on: 746745
Attachment #615950 - Attachment description: (AAv1) Add new default pref value to SeaMonkey too, Remove extra whitespaces. → (AAv1) Add new default pref value to SeaMonkey too, Remove extra whitespaces [Superseded by bug 746745]
Attachment #615950 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.