In Bug #920510, prefs were added to mean that the selected message could be offset from the top or bottom of the message list. However, it is now possible so make it so that the selected message is not even visible! Some sanity checking is needed. There are two prefs - a top offset and a bottom offset. This makes this rather more complicated than just one pref, because it's not exactly clear how the two of them should interact. Here's my suggestion: When moving down, the bottom offset prevails. When moving up, the top offset prevails. If the prevailing offset value would mean that the selected message is off the screen, then reduce the offset until it is just on the screen. Other proposals welcomed. Gerv

I've worked on this some on my own recently, and I had trouble deciding how the two prefs should interact myself. What I eventually came up with is attached; it which seems to work more or less ideally in testing, I'm just not sure how well it would perform on a lower-end machine (my own being kind of ridiculous). The basic idea should be obvious: if the sum of the two prefs is greater than the number of visible rows, repeatedly decrement whichever pref is greater until that isn't true anymore. The benefit of this is that it retains whatever balance between the prefs the user may have set; if one was much greater than the other, it retains as much of that as possible, and if the two were nearly (or exactly) equal, that is retained as well. Does this look like a suitable approach? It doesn't adapt to prefer one pref over the other based on the direction you're moving, but I think that would be a new feature in its own right, not just sanity checking per se. (I don't like open-coding this in both places, but I couldn't find a good or clean way to move it to a function, since I need to modify one or both padding values in the function and then use the modified values in the calling environment; JS doesn't seem to support call-by-reference well, if at all.)

Oops. I think that patch was made from a tree where the current patch for bug 950455 has already been applied. Since these two bugs involve the same area of code, the patch for one is always going to either depend on or conflict with the patch for the other. It shouldn't be too hard to put together a version of this patch that doesn't depend on that one, if needed. Also, now that I've had time to look at it a bit more closely, I don't think "when moving (up|down), the (top|bottom) offset prevails" can even be implemented without a fairly serious redesign; these functions don't have any way of knowing whether you're moving up or down, they only know what message you're trying to move to. It probably wouldn't be impossible to arrange for the needed directional information to get passed in here, but unless it's already available as a global or via a function we could call, doing that would seem to be far beyond the scope of just implementing sanity checking.

> The basic idea should be obvious: if the sum of the two prefs is greater > than the number of visible rows, repeatedly decrement whichever pref is > greater until that isn't true anymore. That sounds like a good algorithm. Let's see how it works in practice. :-) Gerv

(In reply to Gervase Markham [:gerv] from comment #3) > > The basic idea should be obvious: if the sum of the two prefs is greater > > than the number of visible rows, repeatedly decrement whichever pref is > > greater until that isn't true anymore. > > That sounds like a good algorithm. Let's see how it works in practice. :-) > > Gerv To me it sounds like a so so algorithm. When you have something like padding.top 8 and padding.bottom 16 you might end up with padding.top 1 and padding.bottom 8, instead of something close to the original 1:2 like padding.top 3 and padding.bottom 6...

Onno: then why don't we replace the two prefs with a single padding-bottom (or padding-top) value, expressed as a percentage? That way, no more duelling preferences. Gerv

Gerv: Perhaps because the use case of wanting maximum padding in whichever direction is "new messages" without need for much padding in the opposite side seems likely and reasonable (at least to me), but that direction will be different based on sort order? (In reply to Onno Ekker [:nONoNonO UTC+1] from comment #4) > To me it sounds like a so so algorithm. When you have something like > padding.top 8 and padding.bottom 16 you might end up with padding.top 1 and > padding.bottom 8, instead of something close to the original 1:2 like > padding.top 3 and padding.bottom 6... That's a potential flaw, I'll admit, depending on what the user who set those values actually wanted. We could avoid it without dropping separate top/bottom configurability by doing something like: ratio = top / bottom; while (top + bottom > span) { if (top / bottom > ratio) top--; else bottom--; } but I'm not 100% confident that that will work as expected, and it would have edge cases of its own. For example, if you start with a 20-row display and you want at least 5 visible at the top and then as many as possible (15) at the bottom, then transitioning that to a 10-row display would result in 2 and 8 - which preserves the ratio, but loses the "at least 5". Which way is better really depends on whether the user wanted the ratio, or the "absolute minimum distance from one edge". The algorithm in the current patch assumes the latter, and we could do the former with the one above. The question is which one is more likely to fit people's expectations ("principle of least surprise" again) and/or needs in the real world, since providing both based on another new pref seems like overkill.

I think the old threadpane sanitizer extension used only one value to specify when to scroll and one to specify where to scroll to, independent of being at the top or bottom. https://bitbucket.org/martinp26/threadpanesanitizer Anyway, the discussion is maybe a bit academic, because the algorithm only kicks in when there are fewer lines available than padding.top + padding.bottom. Why would a user specify a larger padding than the available size? The only reason I can think of is when changing screen resolution or changing the size of the Thunderbird application window...

Changing screen resolution (repeatedly, when attaching/detaching a laptop from an external monitor) was exactly Gervase's use case for this, IIRC. This bug is about adding sanity checking for when the padding does exceed the available size, whether for good reason or for bad. You're right that we might not need to care overly much about optimizing the behavior for that case, since it's going to be the exception rather than the rule anyway.

Do we really need two prefs for this in the first place? One pref (or maybe even no prefs) might be sufficient. Maybe, instead of using prefs to tweak this, we should just make it easy for extension authors to customize the behavior (and also decide on what we think is a good "baseline" behavior to include in TB).

Originally, there were no prefs; instead, there were two hardcoded constants, both defined to 1. When I made the padding configurable under bug 920510, I kept the separation between top and bottom padding values, and put them in prefs rather than hardcoding them. It makes sense to me to have the values be separate, but I don't have specific use cases for keeping them separate (unless Gervase's is one), beyond the speculative suggestion I've already given. I wouldn't be adamantly opposed to making it easy to configure this sort of thing via an add-on, and removing built-in customizability entirely (although I'd kind of prefer to keep it; my standards for what must/should go in an add-on rather than in the core are somewhat at odds with those of many more-established Mozilla developers). Did you have an approach in mind for doing so? If we could get the two padding values from a single function call (e.g. using some form of call-by-reference, whatever it takes to do that cleanly in JS), we could just have that function return the pre-920510 hardcoded value of 1 for both values in core code, and let add-ons monkeypatch it out with whatever they like - including something like the logic in my current patch, if they want it. However, I haven't been able to find even a really functional - much less a clean - way of doing that. If we move to only a single padding value (used both top and bottom), we could do something similar; it would provide less potential customizability for the add-ons, but it would still work for my use case. I don't know whether it would work for Gervase's, however; as I understand it, he wants sizable (but sanity-checked) bottom padding to provide "look-ahead" visibility when moving through the folder pane, and I don't know whether also using the same padding on the top would work well for him.

(In reply to Andrew Buehler from comment #10) > Originally, there were no prefs; instead, there were two hardcoded > constants, both defined to 1. Right. > I wouldn't be adamantly opposed to making it easy to configure this sort of > thing via an add-on, and removing built-in customizability entirely > (although I'd kind of prefer to keep it; my standards for what must/should > go in an add-on rather than in the core are somewhat at odds with those of > many more-established Mozilla developers). Did you have an approach in mind > for doing so? I was thinking we could turn all of this into a separate function that add-ons could override: http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js?mark=2390-2413#2390 (note that the highlights will be on the wrong lines if you're looking at this from the distant future!) This hypothetical function would return the value we currently hold in |target| (or it would return null, in which case we'd do nothing). I think we could remove the prefs from Thunderbird entirely and instead make the thread pane scroll so that the selected item is always in the middle 50% of the pane. That works for any resolution, gives a fair amount of context, and doesn't cause the thread pane to scroll *every* time you select a new message. Here's a slightly more-advanced option that might work even better. Like above, we want to keep the selected message in the middle 50% of the pane. However, when you select the message below your current one and it's outside of the middle 50%, scroll so that the selected message is near the top (25% down). Basically, when the function scrolls the thread pane, you'd be scrolling by one-half of a page each time. That might make it easier to keep track of where you are, since you're not causing the thread pane to scroll as often. Finally, if this were all a separate function, add-ons could adjust the scrolling behavior however they like, so anyone with more esoteric needs could change the behavior to their heart's content (assuming they can program). That's flexible, while still keeping the maintenance burden on Thunderbird low. > If we could get the two padding values from a single function call (e.g. > using some form of call-by-reference, whatever it takes to do that cleanly > in JS) It probably doesn't matter given my comments above, but this is easy: function foobar() { return [0, 1]; } let [foo, bar] = foobar(); See here for more info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7#Destructuring_assignment_%28Merge_into_own_page.2Fsection%29

I am (In reply to Andrew Buehler from comment #10) > If we move to only a single padding value (used both top and bottom), we > could do something similar; I am not suggesting using a single value for both top and bottom, I am suggesting we change to a percentage value. A percentage value, by its nature, defines a distance from both top and bottom. If the pref is "bottom-padding", and the value is "35%", then clearly the selected message will be 35% from the bottom and 65% from the top, and this will be the case however large or small the thread pane is, without any conflict or confusion. I would rather not move to a system where you need an addon to do monkeypatching; this function is already built into the core, and should stay there. Gerv

(In reply to Gervase Markham [:gerv] from comment #12) > I would rather not move to a system where you need an addon to do > monkeypatching; this function is already built into the core, and should > stay there. I agree that ideally this should stay in the core. However, although I'm strongly opposed to monkeypatching in many cases, this is one case where I think we could design things such that it might be reasonable. If nothing else, keeping this in the core but making it easy to cleanly monkeypatch it out for people who want different behavior would be worthwhile. (In reply to Jim Porter (:squib) from comment #11) > (In reply to Andrew Buehler from comment #10) > > If we could get the two padding values from a single function call (e.g. > > using some form of call-by-reference, whatever it takes to do that cleanly > > in JS) > > It probably doesn't matter given my comments above, but this is easy: > > function foobar() { > return [0, 1]; > } > let [foo, bar] = foobar(); Thanks; I looked around a fair bit for that, but couldn't find any sign of it. Here's a revised patch (using the current pref-based algorithm, rather than switching to a hardcoded value) which moves the get-padding-sizes logic to a function. That would let add-ons override it easily, without touching the rest of the code. This isn't ideally clean, since it duplicates a check and a "get value" from the calling environment, but an add-on replacement would have to do that anyway; a core-code "just return a constant" version wouldn't need to do it, and avoiding it would require arguments that that version wouldn't need. Note that in order to use this same logic in an add-on, you'd have to be able to access FolderDisplayWidget.treeBox from the add-on. I believe that's possible, but I haven't confirmed it; if it isn't, that would be an obstacle we should eliminate before taking the "do it in an add-on" approach. > > I wouldn't be adamantly opposed to making it easy to configure this sort of > > thing via an add-on, and removing built-in customizability entirely > > (although I'd kind of prefer to keep it; my standards for what must/should > > go in an add-on rather than in the core are somewhat at odds with those of > > many more-established Mozilla developers). Did you have an approach in mind > > for doing so? > > I was thinking we could turn all of this into a separate function that > add-ons could override: > http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay. > js?mark=2390-2413#2390 (note that the highlights will be on the wrong lines > if you're looking at this from the distant future!) > > This hypothetical function would return the value we currently hold in > |target| (or it would return null, in which case we'd do nothing). That would still seem to require the monkeypatching add-on's replacement function to duplicate most of the code from this function, if all they want to do is modify the padding value. It also wouldn't cover the padding case for ensureRowRangeIsVisible(), which currently uses the same padding values but different target-calculation logic. Making the target-calculation logic a separate function would indeed improve addon customizability for this (or at least improve the design cleanliness of addons which customize it), but since we'd need two separate functions for the two callers, I'm not sure it makes sense overall. Just moving the "get both padding values" logic into a separate function (and potentially dropping all logic from it in the core, returning a hardcoded value) would let us cover both cases, and also allow add-ons to replace that function without actually copying from it, which I think is cleaner. > I think we could remove the prefs from Thunderbird entirely and instead make > the thread pane scroll so that the selected item is always in the middle 50% > of the pane. That works for any resolution, gives a fair amount of context, > and doesn't cause the thread pane to scroll *every* time you select a new > message. Unfortunately, that approach would not work for the use case which is the reason I wanted to make the padding configurable in the first place. If we do move the target-calculation logic into one or more separate functions, I could probably write an add-on that would replace that logic wholesale and do what I need, but as I said I'm not sure it makes sense to do that move. For my use case, I need a padding of exactly 0, in both directions. Forcing the combined top and bottom padding to equal the current visible rows (either by forcing them equal, or by Gervase's more-configurable percentage-based proposal) would make that impossible. I also don't want to remove the ability for users who like the current "1 row in each direction" padding to keep it. Making the "get padding size" logic easily addon-replaceable, while keeping the current target-calculation logic, would make writing an addon that addresses both of those cases fairly easy. Making the target-calculation logic easily addon-replaceable, while switching the in-core behavior to something like what you describe, would make the needed addon more complicated and harder to maintain. I'm not (yet) convinced the benefit of the new default behavior would be enough to be worth even the trouble of writing it, much less also the trouble of writing the add-on(s) to override it. > Finally, if this were all a separate function, add-ons could adjust the > scrolling behavior however they like, so anyone with more esoteric needs > could change the behavior to their heart's content (assuming they can > program). That's flexible, while still keeping the maintenance burden on > Thunderbird low. I agree that making this easily addon-replaceable would be good. I just think that's orthogonal to the question of making the padding size(s?) easily addon-replaceable. I think the approach from the attached patch does the latter, which would at least be an improvement. I also think that making the target-calculation logic easily addon-replaceable is orthogonal to the question of changing the default target-calculation logic to what you describe. And all of those are arguably only tangentially related to the question of sanity-checking the padding sizes, which is what this bug was originally about...

Comment on attachment 8367900 [details] [diff] [review] thunderbird-threadpane-seeking-padconfig-sanitychecks-shrinkloop-v3.diff Review of attachment 8367900 [details] [diff] [review]: ----------------------------------------------------------------- How about we change mail.threadpane.padding.top/bottom to be percentages as suggested? That should solve everyone's problems. I really don't think we need any prefs here, let alone two prefs, but I guess I can live with them sticking around. I'd go with a default padding of somewhere between 10%-25%. Then you could translate that to a number of rows by something like Math.ceil(topPaddingPercentage / 100 * numberOfVisibleRows). Maybe we should change the names of the prefs, and we can argue about whether the value should be from 0-1 or 0-100... ::: mail/base/content/folderDisplay.js @@ +2321,3 @@ > */ > + getVisibleRowPadding: > + function FolderDisplayWidget_getVisibleRowPadding() { We can change this to a getter now, which looks nicer: get visibleRowPadding() {

(In reply to Jim Porter (:squib) from comment #14) > How about we change mail.threadpane.padding.top/bottom to be percentages as > suggested? That should solve everyone's problems. I can do that, sure. It doesn't strike me as ideal, but that's just my preference for being (able to be) precise speaking; I can see the argument for its being a better default behavior. I do think it would properly belong in a bug (and commit) of its own, but we can do it here if that's what the decision is. > I really don't think we > need any prefs here, let alone two prefs, but I guess I can live with them > sticking around. If we don't keep two values (whether prefs in the core or not), I don't think I see any way to satisfy both my "no padding" use case and the "as much look-ahead as possible" use case which I think Gervase wanted, and which seems reasonable to me. If we do use two values, unless we hard-code them in the core and leave any configurability to add-ons, we have to sanity-check them. There have been only two proposals here for how to do that sanity checking, AFAICT: the one implemented in my patches so far, which tries to retain the exact value of at least the smaller value if possible, and one which instead tries to retain the ratio between the two values. (The other proposal I see was to switch to one single percentage-based pref, and implicitly set the padding for the other side so that the two padding values together make 100%. That one won't allow for my no-padding use case.) Is the current patch's approach to sanity-checking the values acceptable (at least if converted to apply to percentages)? If not, what approach would you prefer? > ::: mail/base/content/folderDisplay.js > @@ +2321,3 @@ > > */ > > + getVisibleRowPadding: > > + function FolderDisplayWidget_getVisibleRowPadding() { > > We can change this to a getter now, which looks nicer: > > get visibleRowPadding() { I agree that that looks nicer, but at a glance and a quick search I'm not finding an obviously functional way to override a getter (monkeypatch it out) as can be done with the 'member function' we currently use. Since part of the appeal of moving this entirely into the separate function is making it easy for addons to replace it, I'd be hesitant to change to a getter unless I understand how addons would do that.

(In reply to Andrew Buehler from comment #15) > (The other proposal I see was to switch to one single percentage-based pref, > and implicitly set the padding for the other side so that the two padding > values together make 100%. That one won't allow for my no-padding use case.) That's not what I meant: I'm suggesting a single percentage padding that is applied to the top and bottom. So for your case, you'd set the pref to 0% (i.e. only scroll when the newly-selected item is actually offscreen). For Gerv's case, he'd set the pref to, say, 50% (i.e. always try to keep the currently-selected item in the middle).

Oh, and here's how you override the getter: Object.defineProperty(FolderDisplayWidget.prototype, "visibleRowPadding", { get: function() { /* blah */ } });

I do not want the currently-selected item in the middle. If I were to write the algorithm just for me, I'd say I'd want it always to be about 25% of the way down. Or, if it were in terms of lines, always at least 10 lines from the bottom, but without ever being off the top of the screen. Gerv

To add to that: we are spending more time on this feature than it's worth, given the other things which need doing on Thunderbird. Let us implement whichever algorithm is easiest to get the current message back on screen. If we can eliminate one of the two prefs at the same time and it's easy, do that. If we can change the remaining pref to a percentage also and it's easy, let's do that. But let's do something, and move on :-) Gerv

(In reply to Jim Porter (:squib) from comment #16) > (In reply to Andrew Buehler from comment #15) > > (The other proposal I see was to switch to one single percentage-based pref, > > and implicitly set the padding for the other side so that the two padding > > values together make 100%. That one won't allow for my no-padding use case.) > > That's not what I meant: I'm suggesting a single percentage padding that is > applied to the top and bottom. Yes, I know. Gervase did suggest that (comment 12), though, which is what I was referring to. (In reply to Gervase Markham [:gerv] from comment #19) > To add to that: we are spending more time on this feature than it's worth, > given the other things which need doing on Thunderbird. Let us implement > whichever algorithm is easiest to get the current message back on screen. Agreed. > If > we can eliminate one of the two prefs at the same time and it's easy, do > that. If we can change the remaining pref to a percentage also and it's > easy, let's do that. But let's do something, and move on :-) I think changing the two prefs to a percentage should be fairly easy, and I'll try to get that done today if other activities permit. I haven't been able to come up with a single-pref design which would cover all known use cases, though. If no one speaks up with a more preferable suggestion in the meantime, I'll probably stick with the existing "shrink the maximum" sanity-checking algorithm.

(In reply to Andrew Buehler from comment #20) > If no one speaks up with a more preferable suggestion in the meantime, I'll > probably stick with the existing "shrink the maximum" sanity-checking > algorithm. If you're using percentages, you should be able to just scale them in unison. For instance, if the paddings are 40% and 80%, the sum of those is 120%, so divide each by 1.2 and you get 33% and 66%. (The math is a little more complex than this since you want the paddings to sum up to less than 100% to allow space for at least one row in between, but that shouldn't be too hard to calculate).

(In reply to Jim Porter (:squib) from comment #21) > If you're using percentages, you should be able to just scale them in > unison. For instance, if the paddings are 40% and 80%, the sum of those is > 120%, so divide each by 1.2 and you get 33% and 66%. That works, but while testing it I noticed a (possibly incidental) problem caused by using ceil() when converting percentages to rows. At least in the attached patch, with those example percentages and a pane height of 10, you get intermediate row values of 3.333... and 6.666..., which become 4 and 7 - greater than the limit. With the additional correction done in the patch, I haven't noticed behavior issues in actual testing. It could be that I just haven't found the right test scenario, though. > (The math is a little > more complex than this since you want the paddings to sum up to less than > 100% to allow space for at least one row in between, but that shouldn't be > too hard to calculate). I've handled it with a simple "if the post-conversion-to-rows values sum to greater than the pane height, decrement whichever is greater". Not entirely ideal, but it's simple enough and it seems to work.

Comment on attachment 8368707 [details] [diff] [review] thunderbird-threadpane-seeking-padconfig-sanitychecks-percentageprefs-proportionalscaling-v1.diff Review of attachment 8368707 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/app/profile/all-thunderbird.js @@ +295,5 @@ > // When using commands like "next message" or "previous message", leave > +// at least this percentage of the thread pane visible above / below the > +// selected message. > +pref("mail.threadpane.padding.top", 10); > +pref("mail.threadpane.padding.bottom", 10); You can't convert these prefs from numbers to percentages without a migration step - can you? I'd expect people to have the old values stored in their profiles, and 3 would no longer be 3 lines, but 3%. Also, if it's a percentage, why can't we just have one pref? 40% from the top implies 60% from the bottom - you only need one number. ::: mail/base/content/folderDisplay.js @@ +2324,5 @@ > + > + // If we can get the height of the folder pane, treat the values as > + // percentages of that, and make sure they don't exceed 100%; otherwise, > + // just use them directly. > + if (this.treeBox) { How does this "if" test relate to what you are actually testing, which seems to be whether to use percentage numbers or not?

(In reply to Gervase Markham [:gerv] from comment #23) > Comment on attachment 8368707 [details] [diff] [review] > thunderbird-threadpane-seeking-padconfig-sanitychecks-percentageprefs- > proportionalscaling-v1.diff > > Review of attachment 8368707 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/app/profile/all-thunderbird.js > @@ +295,5 @@ > > // When using commands like "next message" or "previous message", leave > > +// at least this percentage of the thread pane visible above / below the > > +// selected message. > > +pref("mail.threadpane.padding.top", 10); > > +pref("mail.threadpane.padding.bottom", 10); > > You can't convert these prefs from numbers to percentages without a > migration step - can you? I'd expect people to have the old values stored in > their profiles, and 3 would no longer be 3 lines, but 3%. I thought about that after posting the patch. It's a consideration, but I'm not sure these prefs are yet widely used enough for it to be worth bothering with a migration step; it isn't in the current (ESR-like) release AFAIK, and it hasn't exactly been well-publicized even in the interim versions. > Also, if it's a percentage, why can't we just have one pref? 40% from the > top implies 60% from the bottom - you only need one number. Because, as I've said several times, some use cases - including mine - require padding that does not total 100%. I need 0 padding on both sides; if there were only one pref, that would mean 0% padding on one side would require 100% padding on the other. > ::: mail/base/content/folderDisplay.js > @@ +2324,5 @@ > > + > > + // If we can get the height of the folder pane, treat the values as > > + // percentages of that, and make sure they don't exceed 100%; otherwise, > > + // just use them directly. > > + if (this.treeBox) { > > How does this "if" test relate to what you are actually testing, which seems > to be whether to use percentage numbers or not? That's a side effect, actually. We'd still need paneHeight even if not using percentage numbers (as in the 'shrinkloop' versions of the patch), and we'd still need to fall back to something either way. When using absolute row numbers in the prefs, falling back to the raw pref values made the most sense, from what I could see. When using percentage numbers, falling back to the raw pref values means a change in the semantics of the prefs. That's far from ideal, but the only alternative I can see would be to hardcode a fallback percentage value, which I'm not sure would be preferable. (And I'm uncomfortable enough with the imprecision of the percentage-based approach as compared with absolute row counts, anyway.)

...actually, no; without paneHeight, we couldn't even use a fallback percentage, since there's nothing to calculate percentage *of*. We'd have to hardcode a fallback paneHeight, which is impossible to judge well. The best I can come up with would be to just hardcode absolute row numbers for the fallback case, and ignore the prefs entirely.

Yeah, don't bother migrating the prefs. The old prefs have never been on a release-channel version of TB, so anyone who set the prefs is part of the testing population. A quick note to tb-planning and dev-apps-thunderbird would be polite, though.

Should that note be sent after the patch lands ("this just happened"), or before ("this is about to happen")? The latter would allow additional viewpoints to weigh in if anyone disagrees, but I'm not sure of the proprieties involved. I'm still debating with myself whether to change the names of the prefs, to reflect the change to percentage values. Any arguments for/against that, that I might not have already thought of? The next patch revision is planned to use hardcoded padding of 1 on both sides for the "can't get pane height" fallback case, which should never happen. (I think we should log an error if that does occur, but I'm no longer sure which of the several possible methods I remember existing would be the correct one to use.) Hardcoding 0 would be more robust for the extreme edge case (a pane height of 2 or less), but since that case is so unlikely it's probably not worth bothering. Barring further viewpoints or objections, I think that should leave us with a final patch version, suitable for landing.

(In reply to Andrew Buehler from comment #27) > Should that note be sent after the patch lands ("this just happened"), or > before ("this is about to happen")? The latter would allow additional > viewpoints to weigh in if anyone disagrees, but I'm not sure of the > proprieties involved. After is fine. > I'm still debating with myself whether to change the names of the prefs, to > reflect the change to percentage values. Any arguments for/against that, > that I might not have already thought of? It could be useful to change the names to indicate the units the prefs use (percentages). > The next patch revision is planned to use hardcoded padding of 1 on both > sides for the "can't get pane height" fallback case, which should never > happen. [snip] Components.utils.reportError would work. Also a hardcoded padding of 0 would be fine too. I don't think we'll ever run into that problem, and if we do, we have bigger issues. (But make sure you try this with a standalone message window to make sure that's not affected - it shouldn't be though.)

Sorry for the extended delay, I kept getting distracted from my testing environment. I've now tested with a hardcoded fallback padding of 0, and it seems to work as desired, to the extent that I can test the fallback case at all (which may be not at all). Could you clarify what you mean by "standalone message window"? The only way of producing one that I know of is to double-click on a message in the thread pane, which doesn't affect the thread pane at all. When scrolling with 'f', 'b', or 'n' when the focused window is a message opened in a separate window that way, the displayed focus message in the thread pane does not change, and the thread pane does not scroll. This is appropriate, because the message displayed as focused in the thread pane is still visible in that thread pane's "attached" message pane. This is also the same behavior as without the patch, and the same as in Thunderbird 2. If that's not what you were talking about, then I need clarification of what needs to be tested. With the current patch version, I have only one remaining concern, aside from the imprecision introduced by our somewhat-simplistic rounding: not only are we changing the default behavior, we are not providing any way to get the old behavior back without an add-on which presently does not exist. In the original "make padding configurable" bug and patch, I specifically and intentionally did not change the default behavior (padding of 1 on both sides), even though I don't like that behavior. I would not have seriously objected to changing that default, however, because it would still have been trivial for users who wanted the old behavior to get it back by just changing the prefs. With the change to percentage-based padding, it is no longer trivial to get the old behavior back; doing so would require installing, and in fact at first writing, an add-on to replace the new getter function. I think this is more than is reasonable to expect. If the decision is that changing the default behavior is the right thing to do, then we can of course still do it, but I think that such a change would probably need to be pre-discussed more publicly - just much as was needed for the "remove snap-to-last-page behavior" change, which is if anything less user-visible because it happens less often. Although I can see the arguments for percentage-based padding being a better default, I would personally have preferred to retain a line-based padding behavior in the core, and allow creation of add-ons to enable percentage-based padding of whatever style is desired - specifically because that would let us avoid forcing the changed default behavior on users. (And yes, I do see "use the new behavior or write an add-on" as "forcing" in most cases.)

(In reply to Andrew Buehler from comment #29) > Could you clarify what you mean by "standalone message window"? The only way > of producing one that I know of is to double-click on a message in the > thread pane, which doesn't affect the thread pane at all. That's what I mean. The standalone message window has a subclassed version of the FolderDisplayWidget that you've modified in your patch[1], so you'll need to make sure your patch doesn't break anything. Try opening a standalone message window and moving to other messages in that window with 'f', 'b', etc. Check the error console to make sure your code didn't throw any errors. > If the decision is that changing the default behavior is the right thing to > do, then we can of course still do it, but I think that such a change would > probably need to be pre-discussed more publicly - just much as was needed > for the "remove snap-to-last-page behavior" change, which is if anything > less user-visible because it happens less often. It should be fine to change the behavior, provided we have a default that we're happy with.

Oops, I forgot the link: [1] http://mxr.mozilla.org/comm-central/source/mail/base/content/messageWindow.js#26

Okay. As I mentioned, I've already done that - except for checking the error console - and it worked exactly as I would expect; the visible message in the standalone message window changes appropriately, but the thread pane from which the standalone message window was opened is not affected. I'll check the error console next, but I need to leave for work pretty soon so I may not have the result before tomorrow. Glancing over the code you linked to, I don't see any references (either direct or indirect) to anything I've changed, so it doesn't seem likely anything would be affected in the first place. It's still worth making sure of that, of course.

I've now tested. When moving through messages with 'f' and 'b' while a "standalone message window" is focused, nothing appears in the Error Console.

Ping? Is there anything else I need to do on my end, or is this waiting for someone else to get around to it in some way?

If the patch is ready, flag it for review http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree

Comment on attachment 8377561 [details] [diff] [review] thunderbird-threadpane-seeking-padconfig-sanitychecks-percentageprefs-proportionalscaling-v2.diff I know that's the normal procedure, but in this case it seems for the most part to already have been reviewed by squib, so I wasn't sure that would actually make any difference. Still, indeed it couldn't hurt.

(In reply to Andrew Buehler from comment #36) > I know that's the normal procedure, but in this case it seems for the most > part to already have been reviewed by squib, so I wasn't sure that would > actually make any difference. Still, indeed it couldn't hurt. For tracking purposes it's best to have explicitly set flags for all involved. Bugzilla generates automated reminder e-mails and all of the different dashboard options (both Bugzilla's internal and http://bugmotodo.org/) key off of the flags. If a review? or feedback? flag is not appropriate (although it is in this case, the needinfo flag should ideally be used. If a bug doesn't have one of these set (and isn't assigned to the person whose next action is required), you're depending on that person to have seen the bug, inferred their response is required from the bug, and properly be tracking that their follow-up is required. Speaking as someone who is good at tagging bugmail but not great at going remembering to go look for tagged bugmail, I can personally say I love having explicitly set flags. (And it's still okay to ping too!)

Ping? It's been another two weeks since requesting review on the patch for this, with no reaction. I'm getting a little concerned that this (and bug 950455, which is more important to me and may - based on the discussion which led this bug to be filed in the first place - be informally blocked by this bug) may not make it in in time for the 31 ESR at this point, barring approval of an exception which I'd have a hard time managing to justify. If there's anything for me to do in regard to either bug, I'm on leave all of next week, so I should have plenty of time to work on things.

I'll try to get to this over the weekend. I've been super-busy working on Firefox OS stuff.

I noticed that when you're at the last message and new msgs arrive, they don't scroll into view. Is there already a bug for that or should I file a new one?

That sounds like the behavior which the "snap to last page" behavior (referenced in bug 950455) was meant to be the first step of mitigating, I think as long ago as Thunderbird 3. However, nothing seems to have been done to move further in that direction in the meantime, and given the existence of threaded view it isn't clear what a sane way of doing it would be in the first place. I don't know of any existing bug covering that (though asuth might, if one exists), but as long as you plan to make sure threaded view continues to behave in a sane way and don't plan to remove the ability to have the "scroll automatically" behavior *not* occur, I'd say you should feel free to file one.

Comment on attachment 8377561 [details] [diff] [review] thunderbird-threadpane-seeking-padconfig-sanitychecks-percentageprefs-proportionalscaling-v2.diff Review of attachment 8377561 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this looks ok (I didn't run with the patch, though). I have some comments below that I think should be addressed. Mozmill tests (for the default pref values) would be nice too, if you can manage to get them working. r- for now since I want to take a look at the changes once you've made them. ::: mail/base/content/folderDisplay.js @@ +2352,5 @@ > + if (this.treeBox) { > + topPadding = Math.abs(Services.prefs.getIntPref( > + "mail.threadpane.padding.top_percent")); > + bottomPadding = Math.abs(Services.prefs.getIntPref( > + "mail.threadpane.padding.bottom_percent")); We don't really need to call Math.abs here. In general, we don't try to keep invalid pref values from breaking things, since you shouldn't be manually tweaking prefs unless you know what you're doing. @@ +2358,5 @@ > + // Scale to total at most 100%, barring rounding errors. > + if (topPadding + bottomPadding > 100) { > + topPadding /= (topPadding + bottomPadding) / 100; > + bottomPadding /= (topPadding + bottomPadding) / 100; > + } Likewise here. If someone triggers this with their bad pref values, the onus is on them to fix it. (If we had a UI for these prefs, we'd probably validate the values *before* we store them.) @@ +2366,5 @@ > + let paneHeight = this.treeBox.getPageLength() - 1; > + > + // Convert from percentages to absolute row counts. > + topPadding = Math.ceil((topPadding / 100) * paneHeight); > + bottomPadding = Math.ceil((bottomPadding / 100) * paneHeight); I think it would be clearer if we didn't repurpose these variables. We start out with them as a percentage, and then turn them into row counts. If you don't pay attention to these lines, the following lines would seem confusing.

(In reply to Jim Porter (:squib) from comment #42) > Overall, this looks ok (I didn't run with the patch, though). I have some > comments below that I think should be addressed. > > Mozmill tests (for the default pref values) would be nice too, if you can > manage to get them working. I'll look into that, but since I don't know mozmill properly, it's likely to be enough of a delay that if possible I'd prefer to do it later on a separate bug. That's roughly what happened with the xpcshell tests for the "sort threads by root" option. > We don't really need to call Math.abs here. In general, we don't try to keep > invalid pref values from breaking things, since you shouldn't be manually > tweaking prefs unless you know what you're doing. That's roughly the line of reasoning which was used to argue against adding sanity-checking when this was first made configurable, but since adding sanity-checking is what we're doing here in the first place, it seemed to make sense to do it properly - and of course, before we switched to a percentage-based arrangement, this was an essential *part* of the sanity checking. Will change, though I don't think it's really necessary. > Likewise here. If someone triggers this with their bad pref values, the onus > is on them to fix it. I'm pretty sure I understand the rationale for not doing these checks, but I'm really not comfortable with leaving at least basic sanity checks out - especially since adding sanity-checking is the entire purpose of this bug. Will change, but I suspect I disagree with the governing philosophy about such things. (My own philosophy is roughly centered around "if you're not going to do something right, you shouldn't bother to do it at all"...) > I think it would be clearer if we didn't repurpose these variables. We start > out with them as a percentage, and then turn them into row counts. If you > don't pay attention to these lines, the following lines would seem confusing. I thought of that, but measured against the (admittedly minor) overhead of two additional variables on every pass through, the risk seemed small enough to be worth taking. Still, clearer code is more maintainable, and considering premature optimization and all... Will change. As long as I'm here: should I do a try/catch block rather than "if(this.treeBox) {} else {}"? It's essentially the same pattern, just checking slightly different things.

Sorry about the delay; I'm on the recovering edge of a near-weeklong bout with the flu. I believe this should include the requested changes. Even without clamping the pref values to total at most 100%, the effective padding doesn't let you go outside the visible part of the thread pane in the downward direction (e.g. with top_percent set to 200 and bottom_percent to 0) - but it does in the upward direction (e.g. with the reverse). This seems to be emergent behavior from the ensureRowIsVisible() logic, though I don't 100% understand how it emerges at present. It probably isn't a concern, I just wanted to document it, however cursorily. This also includes the necessary changes to apply now that the patch for bug 950455 has landed.

Comment on attachment 8406363 [details] [diff] [review] thunderbird-threadpane-seeking-padconfig-sanitychecks-percentageprefs-proportionalscaling-v3.diff Review of attachment 8406363 [details] [diff] [review]: ----------------------------------------------------------------- This all looks ok to me now. Thanks for the patch! (And sorry for taking so long on review; moving house is a real pain!)

Thanks. I've been looking into adding mozmill tests for this, as suggested, but I keep getting distracted. If I manage to get something together, I'll file a separate bug.