Fix incorrect window start position #127
Conversation
If the value of `scroll-margin` is not 0, the live buffer won't align with source code buffer properly. Wasn't sure whether to compensate for `scroll-margin` manually, so decided to set it to 0 locally for sake of simplicity.
Codecov Report
@@ Coverage Diff @@
## master #127 +/- ##
=======================================
Coverage 99.28% 99.28%
=======================================
Files 4 4
Lines 985 985
=======================================
Hits 978 978
Misses 7 7Continue to review full report at Codecov.
|
|
Thanks for the commit, I didn't know about Here are the steps I used to reproduce the problem:
Expected result: display stays lined up with the code. Actual result: display is off by 3 lines. |
emacs-live-py-mode/live-py-mode.el
Outdated
| @@ -147,6 +147,7 @@ When the source buffer is narrowed the trace buffer remains | |||
| aligned but will not hide the part after the narrowing." | |||
| (let* ((output-window (get-buffer-window live-py-trace-name)) | |||
| (point-min-pos (point-min)) | |||
| (scroll-margin 0) | |||
There was a problem hiding this comment.
I suggest
(let ((scroll-margin 0)) (recenter-top-bottom 0))
instead to make it clear where the setting is needed.
With Emacs 25.2 and 25.1 (tried no older) I can reproduce the issue and the fix. |
|
@donkirkby Yeah, that is the same issue. After tinkering around some more, I managed to break the commit as well. It seems that instead of using In the af9bd0b commit I also simplified the update scroll function a bit. As for the 85aefe3 commit, it's supposed to be an idea for aligning the source code and output buffer line count, to avoid misaligned code when the source has more lines than output and point is at the end of the buffer. Wasn't sure where to put the code though. |
|
That works for me, now. Thanks. Adding the extra lines broke some of the emacs tests, do you want to fix them, or would you like me to do it? You can see the failures more clearly by running them interactively. There are instructions in the test file, but I ran them like this: If you're not familiar with the test summary (I wasn't), the docs are helpful. There are two more broken tests that have something to do with abandoning an open window. @brandm might be able to clarify why they're failing. |
| (save-restriction | ||
| (widen) | ||
| (line-number-at-pos point-min-pos))))) | ||
| (point-min-line-nr (count-lines 1 (point-min))) |
There was a problem hiding this comment.
Nice solution to compensate for narrowing.
emacs-live-py-mode/live-py-mode.el
Outdated
| (recenter-top-bottom 0) | ||
| (forward-line (- point-line-nr window-start-line-nr))) | ||
| (set-window-buffer output-window live-py-trace-name))) |
There was a problem hiding this comment.
This move breaks
live-py-test/3-other-window/4-abandon-source-and-swap/edit
live-py-test/3-other-window/4-abandon-source-and-swap/move
What issue is the move expected to fix?
There was a problem hiding this comment.
Ah, alright, didn't know it was that important. Just seemed odd to display a buffer that was already checked as visible.
emacs-live-py-mode/live-py-mode.el
Outdated
| @@ -170,7 +165,7 @@ aligned but will not hide the part after the narrowing." | |||
| ;; interactivity when `debug-on-error' is active, so it is easily possible | |||
| ;; to miss the error. And on such an error the function will be removed | |||
| ;; from `post-command-hook' which is confusing when not noticed. | |||
| (when (memq this-command '(narrow-to-region next-line viper-goto-line)) | |||
| (when (memq this-command '(narrow-to-region next-line previous-line viper-goto-line)) | |||
There was a problem hiding this comment.
For performance reasons this list should be as short as possible because it requires a redisplay for reasons explained in the comments. Why add previous-line? If it is just for symmetry with next-line then please remove previous-line again and if it helps add a comment below the comment for next-line that it is not in the list intentionally.
| @@ -207,6 +202,7 @@ aligned but will not hide the part after the narrowing." | |||
| (get-buffer-create live-py-trace-name) | |||
| (with-current-buffer live-py-trace-name | |||
| (setq-local truncate-lines t) | |||
| (setq-local scroll-margin 0) | |||
If the backend would add the extra lines for the line count alignment instead of the Emacs frontend then the other frontends would benefit as well. |
Yes, the expected output buffer |
|
Alright, fixed the mentioned issues and moved the line alignment to the live-py backend. I ran the tests and two of them failed due to the appended lines, but that's obviously intentional in this case. |
|
I fixed the broken unit tests by adding some extra blank lines. Thanks for the fix, @ZarsBranchkin, and thanks for the review, @brandm. |
If the value of
scroll-marginis not 0, the live buffer won't align with source code buffer properly.Wasn't sure whether to compensate for
scroll-marginmanually, so decided to set it to 0 locally for sake of simplicity.