Steps to reproduce: * Scroll to the bottom of the list of messages. * Click on the next-to-topmost visible message. * Press 'b', to trigger the "Previous Message" command. Expected result: * The selected message changes to the topmost visible message, and the message list does not scroll. Actual result: * The message list scrolls so that the newly selected message is no longer the topmost. The reverse behavior, when scrolling downward with the "Next Message" command ('f') from the next-to-bottommost visible message, also holds. Thunderbird 2 did not include this padding; Thunderbird 5 did, and TB3 likely did as well. There's nothing wrong with having it or with its being the default, but it should be optional; for that matter, there might be value in having it be configurable. Implementing that configurability in an add-on, in a way which is robust against unreasonable values for the size of the padding, would seem to require that the add-on include tweaked copies of FolderDisplayWidget.ensureRowIsVisible() and .ensureRowRangeIsVisible(), and monkeypatch them over the originals; given the usual problems with code duplication, this seems like a poor idea. The attached (possibly overengineered) patch makes the padding size configurable, within reasonable limits derived from the size of the visible message list, without adding default about:config entries for the new prefs.

I forgot to explain why I consider this a problem... Short version: It messes up page-by-page message counting when reading through large volumes of unread messages. I have large archives of such messages in E-mail (from the LKML), and I encounter them on Usenet as well; I like to read through them in units of X, and I size the message list to match, so that when I reach the end or beginning of a message-list page I know I'm at a multiple of X. Having the message list scroll so that the selected message is not at the beginning or end of the visible page interferes with that. This is an obscure and arbitrary use case, but it's one I'd be highly uncomfortable without, and even outside the context of that use case this seems to me like something that should be reasonable to configure.

Put it another way: If I select the topmost visible message (no matter how - with the mouse, the arrow keys, or the 'f'/'b'/'n' commands), then scroll up by one page using the scroll bar, I intuitively expect the bottommost visible message to be the one immediately above my currently selected message. (Likewise for the reverse scrolling-down case.) Having the padding in place interferes with that expectation. If page-at-a-time scrolling were fixed to wrangle the padding appropriately to guarantee that, while I wouldn't like it as much as the no-padding-at-all behavior, I could probably adjust to live with it.

Comment on attachment 809853 [details] [diff] [review] thunderbird-messagelist-seeking-padconfig-v3.diff I'm no longer a reviewer for Thunderbird code; I think :mconley may be your best bet. I can however, provide some feedback as the person originally involved in writing this code / porting/documenting what TB2 did while normalizing/cleaning up. First, thank you for your extensive rationale for the change. This is super-useful context! In general, I think this is a reasonable approach. I'm not sure I agree that monkey-patching the ensureRow* methods is so horrible for an extension to do, but since you're just changing the logic related to an arbitrary constant and you're not adding any new notable permutations, this seems reasonable to do in core. Especially if this lures you into additional contributions! :) (You may already be contributing a lot; I'm out of the loop.) Specific notes: - Thunderbird-specific preferences should start with "mail.", not "mailnews.". The existing prefs already exist for legacy purposes. I like the hierarchy/specificity of the prefs, though! - You should add the prefs to mail/app/profile/all-thunderbird.js, near where mail.operate_on_msgs_in_collapsed_threads. Set the default to 1. This will allow you to remove the try/catch blocks. Ideally, provide a brief comment before the pair that notes that the logic clamps them to half the number of visible rows. Maybe also mention that if the default is changed from 1 that the mozmill tests may need to be updated. - The convention I used for THINGS_IN_CAPITALS was constant-ish values. Since you're converting them into helper functions, I'd probably name them something like _computeTopRowPaddingForVisibleRows and _computeBottomRowPaddingForVisibleRows. ('span' in the function was probably a bad name choice on my part.) I would update the comment blocks too; since the name expresses a lot, no need to go crazy. A nice side-effect of the change is that if anyone is monkey-patching just the constants, they will not break things if they keep clobbering the values. - I don't think it's worth worrying about the existing constants otherwise / if there's an extension that clobbers them, it will be mooted. I would send a mail to tb-planning assuming this change lands after it lands as a heads-up to existing extension authors. - Since I was initially concerned about this, it's fine for us to be accessing the preferences in this way. The file was written with no performance concerns about prefs checking (since it is a fast-ish synchronous operation) and the ensure calls happen in a timeout anyways. - I don't think any unit tests/unit test-changes are required since this is a secret pref. - I'm a bit freaked out by the existing code's use of parseInt instead of Math.floor. I have no idea what's up with that. I'm assuming I just ported that logic. Since we're in a Number context there, I think that should be changed to Math.floor while you're touching this code.

> First, thank you for your extensive rationale for the change. This > is super-useful context! Thank *you* for the prompt response! > In general, I think this is a reasonable approach. I'm not sure I > agree that monkey-patching the ensureRow* methods is so horrible for > an extension to do, but since you're just changing the logic related > to an arbitrary constant and you're not adding any new notable > permutations, this seems reasonable to do in core. Especially if > this lures you into additional contributions! :) (You may already be > contributing a lot; I'm out of the loop.) I haven't been contributing a lot, but I've had one patch accepted, and I'm looking into doing more. At the moment I'm focusing on making it possible to restore the things I like and/or rely on from the TB2 UI, but once that's either done or sufficiently rejected, I do plan to branch out into more mainstream TB contributions. > Specific notes: > > - Thunderbird-specific preferences should start with "mail.", not > "mailnews.". The existing prefs already exist for legacy purposes. I > like the hierarchy/specificity of the prefs, though! Okay. My previous contribution did add a mailnews. pref, and I thought it would be appropriate because the behavior controlled by this pref would affect both mail and newsgroups, but I can certainly change this. (I did the "hierarchy" bit because I have another change pending submission, which touches the same area of the code and so would either conflict with or depend on this one, which would fit under the same hierarchy.) > - You should add the prefs to mail/app/profile/all-thunderbird.js, > near where mail.operate_on_msgs_in_collapsed_threads. Set the > default to 1. This will allow you to remove the try/catch blocks. I actually did this originally (using mailnews/mailnews.js, which is where my previous contribution's pref went), then removed it for reasons that no longer make quite as much sense. I'd be glad to change it back. > - The convention I used for THINGS_IN_CAPITALS was constant-ish > values. Since you're converting them into helper functions, I'd > probably name them something like _computeTopRowPaddingForVisibleRows > and _computeBottomRowPaddingForVisibleRows. ('span' in the function > was probably a bad name choice on my part.) Agreed. I left them with the CAPITALIZED names out of a "minimize total changes" idea, and because I wasn't entirely sure what names would be appropriate. ACK on all the rest. I'll work up a second patch once I get back from work. > - I'm a bit freaked out by the existing code's use of parseInt > instead of Math.floor. I have no idea what's up with that. I'm > assuming I just ported that logic. Since we're in a Number context > there, I think that should be changed to Math.floor while you're > touching this code. I'll try to dig into this when I get a chance, and make sure that the end result works the same way as the original.

(In reply to Andrew Buehler from comment #4) > > Specific notes: > > > > - Thunderbird-specific preferences should start with "mail.", not > > "mailnews.". The existing prefs already exist for legacy purposes. I > > like the hierarchy/specificity of the prefs, though! > > Okay. My previous contribution did add a mailnews. pref, and I thought > it would be appropriate because the behavior controlled by this pref > would affect both mail and newsgroups, but I can certainly change this. The name is a legacy thing and doesn't really have anything to do with NNTP/newsgroups. The basic idea is that things in 'mailnews/' in the tree are shared between Thunderbird and SeaMonkey and things in 'mail/' are Thunderbird-specific. The UI bits for SeaMonkey live in 'suite/' with mail stuff living in 'suite/mailnews/'. There is a folderDisplay.js in there, but it's basically a compatibility shim so extensions can work in SeaMonkey too or more directly ported logic can work.

There's a dangling, incomplete sentence in the middle of the comment on ensureRowIsVisible(), about intending to implement some unspecified thing later. I'd like to fix it up as long as I'm in the area. Can you comment on what this may have been intended to refer to, or should I just drop that sentence fragment entirely?

No idea; I'd drop the fragment. Usually a sentence like that is an indication that I realized there was something else I needed to do or that I could address whatever I was commenting about at the time.

Revised patch. I couldn't figure out a suitable way to trim down or clean up the comments on the constants which are now becoming helper functions without just removing them entirely, so I ended up expanding them instead. If there are suggestions for how to trim them down, or if just dropping the comments would be better, I'd be fine with either. When changing from the CAPITALIZED constant names to the suggested helper-function names, I've had to rewrap in some fairly odd ways to avoid excessively long lines. I think this version works, but I don't know what the coding style here is. I renamed the 'span' variable in the helper functions to be more descriptive, but left it alone in the existing functions, because the lines are long enough as it is. I don't know how to trigger ensureRowRangeIsVisible() from the UI, so I haven't been able to test the parseInt() -> Math.floor() conversion, but it looks correct.

(In reply to Andrew Sutherland (:asuth) from comment #3) > - You should add the prefs to mail/app/profile/all-thunderbird.js, near > where mail.operate_on_msgs_in_collapsed_threads. Set the default to 1. > This will allow you to remove the try/catch blocks. Ideally, provide a > brief comment before the pair that notes that the logic clamps them to half > the number of visible rows. Maybe also mention that if the default is > changed from 1 that the mozmill tests may need to be updated. I seem to have completely missed the latter half of this note when revising the patch. Unless suggested otherwise, future versions will include the suggested comments, rather than adding duplicate mentions of clamping in the existing comments.

Hm - I've not dabbled in too much folder display code. While I could attempt to grok what's going on here, you might get a faster review from somebody with more experience (particularly, experience with XUL trees). Magnus, have you worked on folderDisplay.js before?

Comment on attachment 810584 [details] [diff] [review] thunderbird-messagelist-seeking-padconfig-v4.diff Review of attachment 810584 [details] [diff] [review]: ----------------------------------------------------------------- Since I'm still a submodule owner of the 3pane, I'm probably one of the folks who should look at this. I haven't tested this patch yet, but it looks ok for the most part. My comments are below. ::: mail/app/profile/all-thunderbird.js @@ +280,5 @@ > +// hidden prefs for the minimum number of visible rows to leave above/below the > +// selected message in the message list, when using commands like "next > +// message" or "previous message" > +pref("mail.message_list.padding.top", 1); > +pref("mail.message_list.padding.bottom", 1); Nit: I'd call these prefs "mail.threadpane.padding.top"/"bottom". ::: mail/base/content/folderDisplay.js @@ +2324,2 @@ > */ > + computeTopRowPaddingForVisibleRows: We could probably merge this and computeBottomRowPaddingForVisibleRows into a single function that takes a parameter to specify top vs bottom. Maybe name it "computeVisibleRowPadding"? That'd be shorter so you wouldn't have to deal with quite as many line-wrapping issues. @@ +2324,4 @@ > */ > + computeTopRowPaddingForVisibleRows: > + function FolderDisplayWidget_computeTopRowPaddingForVisibleRows(pageLength) { > + return Math.abs(Math.min(Services.prefs.getIntPref("mail.message_list.padding.top"), Why the Math.abs here? @@ +2347,4 @@ > * By padding, we mean that the index will not be the first or last message > * displayed, but rather have messages on either side. > * If we get near the end of the list of messages, we 'snap' to the last page > + * of messages. Nit: remove the trailing whitespace here.

(In reply to Jim Porter (:squib) from comment #11) > Comment on attachment 810584 [details] [diff] [review] > thunderbird-messagelist-seeking-padconfig-v4.diff > > Review of attachment 810584 [details] [diff] [review]: > ----------------------------------------------------------------- > > Since I'm still a submodule owner of the 3pane, I'm probably one of the > folks who should look at this. I haven't tested this patch yet, but it looks > ok for the most part. My comments are below. Thanks. Tangentially, I have another change I'd like to submit which touches this same file (and in fact one of these same functions); although I want it for similar reasons, it's a logically separate change. Would you be an appropriate person to request review from when I submit that? > ::: mail/app/profile/all-thunderbird.js > @@ +280,5 @@ > > +// hidden prefs for the minimum number of visible rows to leave above/below the > > +// selected message in the message list, when using commands like "next > > +// message" or "previous message" > > +pref("mail.message_list.padding.top", 1); > > +pref("mail.message_list.padding.bottom", 1); > > Nit: I'd call these prefs "mail.threadpane.padding.top"/"bottom". Done. There seem to be at least three different names for this section of the UI, and it's not remotely clear which if any is more official - to say nothing of which is "best". (While "thread pane" is at least unambiguous, unlike "message pane", it doesn't make sense for people who don't use threaded mode.) > ::: mail/base/content/folderDisplay.js > @@ +2324,2 @@ > > */ > > + computeTopRowPaddingForVisibleRows: > > We could probably merge this and computeBottomRowPaddingForVisibleRows into > a single function that takes a parameter to specify top vs bottom. Maybe > name it "computeVisibleRowPadding"? That'd be shorter so you wouldn't have > to deal with quite as many line-wrapping issues. I like the idea, but that would involve either using a magic number for the parameter (with meaning defined in the function comment), or adding in DEFINED_CONSTANTS like the ones from the original code - which latter would eliminate the line-length savings. I can see where the former might be acceptable; see revised patch for an attempt to do it without code duplication. I could reduce line length further (to below the 80-column limit) by adding a local variable to hold the pref name, but I'm not sure which side to err on there. > @@ +2324,4 @@ > > */ > > + computeTopRowPaddingForVisibleRows: > > + function FolderDisplayWidget_computeTopRowPaddingForVisibleRows(pageLength) { > > + return Math.abs(Math.min(Services.prefs.getIntPref("mail.message_list.padding.top"), > > Why the Math.abs here? Because otherwise this would happily accept negative values, resulting in odd behavior; negative padding sizes don't really make sense. I actually addressed this in the relevant comment while waiting for review, I just didn't post a new patch for it. Omitting the Math.abs would reduce line length by a small but important factor, but I'm not sure it's worth it. (I could be persuaded that we should allow people to configure negative padding sizes if they want to, but the odds of someone wanting to seem much lower than the odds of someone getting lost and upset because something introduced a negative value for one of the prefs.) Looking at this again, I've noticed that taking the absolute value of the Math.min() is actually a bug; a negative value whose absolute value was greater than Math.floor(pageLength/2) would result in padding larger than the intended maximum. The revised patch takes the absolute value of the actual pref itself instead. > @@ +2347,4 @@ > > * By padding, we mean that the index will not be the first or last message > > * displayed, but rather have messages on either side. > > * If we get near the end of the list of messages, we 'snap' to the last page > > + * of messages. > > Nit: remove the trailing whitespace here. Done. That was an oversight, sorry.

Comment on attachment 817158 [details] [diff] [review] thunderbird-messagelist-seeking-padconfig-v5.diff Review of attachment 817158 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/app/profile/all-thunderbird.js @@ +292,5 @@ > pref("mail.warn_on_shift_delete", true); > > +// hidden prefs for the minimum number of visible rows to leave above/below the > +// selected message in the thread pane, when using commands like "next message" > +// or "previous message" Nit: make it a proper sentence with Capital start and full stop. ::: mail/base/content/folderDisplay.js @@ +2329,5 @@ > + computeVisibleRowPadding: > + function FolderDisplayWidget_computeVisibleRowPadding(padEnd, pageLength) { > + return Math.min(Math.abs(Services.prefs.getIntPref("mail.threadpane.padding." + > + (padEnd ? "top" : "bottom"))), > + Math.floor(pageLength/2)); You don't need the Math.min - we generally don't support entering junk values in prefs. @@ +2376,5 @@ > let span = treeBox.getPageLength() - halfVisible; > > let target; > // If the index is near the end, try and latch on to the bottom. > + if (aViewIndex + span - this.computeVisibleRowPadding(1, span) > parenthesis around this stuff would make it easier to read

let squib review, as he already started

(In reply to Magnus Melin from comment #13) > Comment on attachment 817158 [details] [diff] [review] > thunderbird-messagelist-seeking-padconfig-v5.diff > > Review of attachment 817158 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/app/profile/all-thunderbird.js > @@ +292,5 @@ > > pref("mail.warn_on_shift_delete", true); > > > > +// hidden prefs for the minimum number of visible rows to leave above/below the > > +// selected message in the thread pane, when using commands like "next message" > > +// or "previous message" > > Nit: make it a proper sentence with Capital start and full stop. Can do (though rephrasing to actually let it be grammatical might be a bit tricky), but given that at least two other pref comments in this file and several in mailnews/mailnews.js use this exact form, is this really necessary? I chose this form at least in part because it matches what other pref comments were already doing. I don't have a particular problem with changing it, but I'd at least like to know what rules I'm trying to follow here. > ::: mail/base/content/folderDisplay.js > @@ +2329,5 @@ > > + computeVisibleRowPadding: > > + function FolderDisplayWidget_computeVisibleRowPadding(padEnd, pageLength) { > > + return Math.min(Math.abs(Services.prefs.getIntPref("mail.threadpane.padding." + > > + (padEnd ? "top" : "bottom"))), > > + Math.floor(pageLength/2)); > > You don't need the Math.min - we generally don't support entering junk > values in prefs. In that case, there isn't really any point to turning these into functions; the only reason I did that was to allow passing in the pageLength value, which is only used in one side of Math.min(). If we aren't going to be robust against unreasonable pref values (in which case we can drop the Math.abs() as well), then it should work just as well to just redefine the original *_VIEW_PADDING constants to the results of getIntPref() calls. (Which is what I originally did, before noticing the consequences of unreasonable pref values.) At which point, the only change left in the two ensure*IsVisible functions should be the "fix it while we're here anyway" change from parseInt() to Math.floor(). With the total non-incidental change that minimal, at that point it really would probably make more sense to do this in an add-on, since there's no need to monkeypatch those two functions anymore, just a pair of constants. Of course, if I were implementing this as an add-on, I'd want to be robust against unreasonable pref values - which reintroduces the monkeypatching, and leaves us right where we started again... > @@ +2376,5 @@ > > let span = treeBox.getPageLength() - halfVisible; > > > > let target; > > // If the index is near the end, try and latch on to the bottom. > > + if (aViewIndex + span - this.computeVisibleRowPadding(1, span) > > > parenthesis around this stuff would make it easier to read I'm not quite sure I follow. Are you suggesting + if ((aViewIndex + span - this.computeVisibleRowPadding(1, span)) > ? Because that's the only place I can see where parentheses would seem to logically fit... and I thought the line breaks made the grouping clear enough already. At least at a glance, I'm pretty sure I think reading this with those parentheses is harder than without, in this particular case. (Some of the other cases might be a different story. I'd have to try it out.)

Re sentences: if it looks like a sentence, make it proper. Just because it's a small change doesn't mean it needs to be in an add-on, if it's generally useful. Yes, that's what i'd suggest regarding the parenthesizes.

To me, though, the comment that's there *doesn't* look like a sentence - and neither do the others of the same form; they look like noun-phrase sentence fragments, and I'm pretty sure they were done that way intentionally for conciseness (though I'll admit this one isn't as concise). To make it look like a sentence and still fulfill its function as a comment, I'd have to rephrase somewhat nontrivially, something like: +// When using commands like "next message" or "previous message", leave +// at least this many visible rows above or below the selected message +// in the thread pane. Although I can certainly use this form if the reviewers prefer it, to my eye, the minor circumlocutions necessary to introduce a verb and make this flow grammatically make it harder to instantly pick up what the comment is explaining. Without those changes, this just isn't a sentence. (Please understand: I'm entirely in favor of properly punctuating, capitalizing, and otherwise polishing comment sentences. I just don't think this is a sentence, and thus don't think it needs it.) The advantage of having it in an add-on would be that it could more readily be used on older versions (such as the recently-released TB24) as well. I didn't think that was worth the bad practice of monkeypatching functions without really changing their logic (I'd just have had to manually modify omni.ja in my local TB24 installs until this made it into a release version), but without that disadvantage, the decision might fall the other way. Acknowledged on the parentheses. I'd have a harder time reading this example with them than without them; with two open-parens at the beginning of the line and two close-parens at the end, the intuitive interpretation is that they're matched with each other. To make completely sure of which parentheses really do match with each other, I'd now have to manually count opens vs. closes. However, I'm aware that not everyone else is like me in every respect, and I can bow to the maintainers' decision.

Certainly, feel free to add verbs as needed. Your suggestion is much better than the original proposal.

I actually think it looks worse (and out of sync with many/most of the other comments in the file), hut it's not worth the bother for me to argue about it. Since you've given at least as much feedback/review on this so far as :squib has, I've transferred the review request over to you for this latest patch; if that was the wrong thing to do, please feel free to either transfer it back, or let me know and I'll do it. I think I've addressed all your suggestions, though IMO the code is less readable with the extra parentheses. I've considered adding local variables to each ensureRow* function to let us get the pref only once for each; that would not only avoid the need for a helper function entirely, let us reduce line length further, and improve performance very slightly due to fewer actual pref reads, but should reduce the readability isssues I see, by eliminating the function-call parentheses at every place where the pref is used. Does that seem reasonable?

Comment on attachment 819121 [details] [diff] [review] thunderbird-messagelist-seeking-padconfig-v7.diff Review of attachment 819121 [details] [diff] [review]: ----------------------------------------------------------------- Thx for the patch, Andrew. r=mkmelin with a few nits Please fix a few style nits, add the review comment to the commit message, and then add the checkin-needed keyword. ::: mail/app/profile/all-thunderbird.js @@ +291,5 @@ > pref("mail.warn_on_collapsed_thread_operation", true); > pref("mail.warn_on_shift_delete", true); > > +// When using commands like "next message" or "previous message", leave > +// at least this many visible rows above or below the selected message remove trailing whitespace on both lines above ::: mail/base/content/folderDisplay.js @@ +2314,5 @@ > */ > //@{ > > /** > + * Minimum number of lines to display between the 'focused' message and the remove trailing whitespace @@ +2317,5 @@ > /** > + * Minimum number of lines to display between the 'focused' message and the > + * top or bottom of the thread pane. > + * If the default pref value gets changed from 1, tests may need to be > + * updated. you can remove that comment, it doesn't belong in a function documentation @@ +2320,5 @@ > + * If the default pref value gets changed from 1, tests may need to be > + * updated. > + * > + * @param padEnd 1 to get padding at the top of the pane, 0 to get padding at > + * the bottom of the pane. "number of padding rows at the top of the pane" maybe? @@ +2325,3 @@ > */ > + getVisibleRowPadding: > + function FolderDisplayWidget_getVisibleRowPadding(padEnd) { there's a convetion to prefix the parameters with a, like aPadEnd @@ +2325,5 @@ > */ > + getVisibleRowPadding: > + function FolderDisplayWidget_getVisibleRowPadding(padEnd) { > + return Services.prefs.getIntPref("mail.threadpane.padding." + > + (padEnd ? "top" : "bottom")); would prefer Services.prefs.getIntPref(aPadEnd ? "mail.threadpane.padding.top" : "mail.threadpane.padding.bottom"); ... for better searchability @@ +2432,5 @@ > let target; > // if the range is bigger than we can fit, optimize position for the min row > // with padding to make it obvious the range doesn't extend above the row. > if (aMaxRow - aMinRow > span) > + target = Math.max(0, (aMinRow - this.getVisibleRowPadding(1))); while you're here, please add braces to this if clause. (the style guide we follow is that if one of the if-elses have braces, they all should)

(In reply to Magnus Melin from comment #20) I've addressed all comments, and a revised patch will follow. While addressing the comments, I noticed a misplaced close-paren which looked like it should introduce a bug, but testing showed it works fine anyway. Investigating this, I discovered that our Math.min()/Math.max() calls are actually redundant; they're intended to make sure we don't scroll above the top or below the bottom of the message list, but scrollToRow() already ensures that. Is it worth fixing that while we're here (with another review iteration), or should we just proceed with what we have? If I don't hear back on this within a few days to a week, I'll leave it alone and post the new patch with your r+ as directed. > while you're here, please add braces to this if clause. (the style guide we > follow is that if one of the if-elses have braces, they all should) That makes sense, but in this case, it introduces some confusion with regard to the comment which follows. That comment currently precedes the 'else' line, but refers to the contents of the 'else' clause. Should it be moved to after the 'else' (and reindented/rewrapped), or should the new } be on a line by itself preceding the comment, or should the comment be left inside curly-brace for the half of the 'if' clause it does not refer to? I've taken that last option for now, as the one which changes the fewest lines, but I'm not at all sure it's the right one.

Yes, please remove the redundant min/max I'd move the comment inside the else clause, like else { // So the range..........

I was wrong, they're not redundant. Without them and with padding of at least 1, there's an unlikely edge case that can trigger undesired behavior. (TL;DR: I've kept the min/max calls. The rest of this is just explaining the problem and how to trigger it, for documentation in case someone else later thinks the same thing I did.) I also have another new pref pending submission (which I haven't included in these patches as it's logically unrelated despite being in the same code). With that pref and a padding of at least 2, dropping the min/max calls would produce the undesired behavior every time you switch to a new folder. The undesired behavior is simply displaying additional blank rows above or below the topmost/bottommost message in the thread pane, when scrolled to the top/bottom of the pane. The unlikely edge case is to: 1. Set padding to at least 1. 2. Scroll to the top/bottom of the thread pane. 3. Select the topmost/bottommost visible message. 4. Scroll so that that message is no longer visible. 5. Press 'b'/'f'. The easy/automatic case is to: 1. Set padding to at least 2. 2. Disable the "snap to last page" behavior (e.g. with my other pending new pref). 3. Switch to a new folder, which has more messages/threads than there are visible rows in the thread pane. With the min/max calls, none of this happens.

Here's the revised patch, which should be final.

https://hg.mozilla.org/comm-central/rev/1c6c0b697be4 Fixed the bug # in the commit message before pushing.

Oops - I double-checked that twice, but apparently it wasn't enough. I'll try to be more careful still next time.