Show beginning-of-defun only if the top of the window is inside a defun

[roshanshariff]

This PR makes two changes to the topsy--beginning-of-defun function:

• It checks if the top of the window is actually inside a defun. If not, it returns nil, which results in a blank header line (but see below).
• It widens the buffer before finding the beginning-of-defun, in case only part of the defun is visible. This is a small change that affects the same code as the rest of this PR, so I decided to include it here.

This is the second in a series of PRs that splits out the functionality of #7. The next PR in this series (#18) makes it possible to use the header line as an extra text line when there is no sticky header, for example when the beginning of the window is not inside a defun. Together, they fix bug #1.

[alphapapa]

Hi Roshan,

I appreciate your attention to detail in splitting up #7 into these PRs.

It checks if the top of the window is actually inside a defun. If not, it returns nil, which results in a blank header line (but see below).

I think that makes sense, yes. It seems more consistent.

It widens the buffer before finding the beginning-of-defun, in case only part of the defun is visible. This is a small change that affects the same code as the rest of this PR, so I decided to include it here.

This might be fine, but I'm not sure if it's the right thing to do. Is it only relevant for a case in which the buffer is narrowed across function boundaries? If so, it might be better to not do this, or to gate it behind an option, because generally it seems like it's better to respect the boundaries of a narrowed buffer. What do you think?

Thanks.

[roshanshariff]

That was actually how I implemented it initially, following the advice to respect the user's narrowing. But sometimes when trying to make sense of a giant function, I'll narrow the buffer to a region of interest. Then Topsy would just show the beginning of the narrowed part as the "defun" even though it's not. For example:

I found this confusing and unnecessary, because it's quite obvious where the visible part begins. It's much more useful to see the defun that the narrowed region belongs to, rather than just the first line of the narrowed region:

I considered making this a customizable option, but after using it for a long time I haven't come across a situation where I wanted to disable this feature. As you say, it only really matters when you've narrowed the buffer down so it doesn't include the beginning of a defun. If you use the much more common narrow-to-defun then you see the first line of the defun as expected. But if you feel it's better, I can gate it behind a defcustom.

[roshanshariff]

Actually, I realized that with the first change, it works reasonably well to not widen the buffer. If the beginning of the narrowed buffer is visible, the header line remains blank. When you scroll down, the header line starts showing the beginning of the narrowed region. The header line never duplicates visible text or shows anything outside the narrowed region.

If you like, I can update the patch to add another defcustom that controls whether (widen) gets called.

[alphapapa]

After thinking about this some more, I think we need to be careful not to cause performance problems. The header line, like the mode line, is updated very often, and we already call beginning-of-defun every time; with this change we'd also call end-of-defun every time. I can imagine enabling this mode and then noticing performance problems when scrolling in large files with long functions or other top-level forms (keeping in mind that this mode isn't just for Elisp files).

So I'm thinking that it might be best to leave the topsy--beginning-of-defun function as-is, and then define another function which could be used instead, one which would provide extra "accuracy" in the way this PR's function does. And then the user could choose between them with an option, which could default to the choice which provides the best performance.

What do you think? Thanks.

[roshanshariff]

Instead of making another function, would it be okay to just add a defcustom topsy-defun-before-end that controls whether to call end-of-defun? The code already handles the case when end-of-defun fails because I encountered this at some point, so this is a minimal change.

I've changed the commit to do this, and also added a defcustom to control whether the buffer is widened. Both options default to nil, so it behaves almost exactly like before. The only change is that if the defun is entirely visible, then the function returns nil to avoid showing the beginning twice.

[alphapapa]

I don't know. I'm reluctant to add more options, because that makes code harder to understand, and it also can make it harder for the user to understand. If we use alternative functions, then users can easily choose a different function that provides a different behavior, as well as providing their own (e.g. I don't know if I want to add all of these things you've proposed to this package, but if we implemented this so that users could just plug in their own function, you could easily have these customizations without having to maintain a fork).

[roshanshariff]

I understand the argument against option fatigue, and I only proposed it because the extra code complexity seemed minimal. And at least in my experience, the mere presence of options hasn't put me off Emacs packages, as long as I don't have to customize them to get a useful result.

The problem with having separate functions is that you get a combinatorial explosion: here are two options, with four possible states; you'd need four functions to get all the behaviours. It seems hard to come up with extension points more fine grained than outright replacing the topsy header function. Of course, one could write a new function that behaves exactly as one wants, but that is much more daunting for many users than checking a box in the Customize interface.

That said, since it is already possible to replace the topsy header function, maybe it's best to close this PR? It seems unnecessary to me to have the package provide multiple functions that are minor variations of each other (one that also checks end-of-defun, another that widens the buffer).

[alphapapa]

I don't think I want to close the PR. Your ideas and code are good, I'm just not yet sure of how I want to fit them into the design of the library as a whole. I want to make it extensible and flexible while also keeping it fairly simple. If you don't mind, I'd like to revisit these PRs in the future and look at them with "fresh eyes."