Skip to content

feat: add bottom_margin config for prompt positioning#115

Open
brancengregory wants to merge 2 commits intoeitsupi:mainfrom
brancengregory:buffer
Open

feat: add bottom_margin config for prompt positioning#115
brancengregory wants to merge 2 commits intoeitsupi:mainfrom
brancengregory:buffer

Conversation

@brancengregory
Copy link
Copy Markdown

@brancengregory brancengregory commented Mar 12, 2026

  • adds BottomMargin enum with Fixed, Proportional, and Disabled variants
  • places config in [editor] section (not [prompt]) for terminal positioning control
  • implements ensure_bottom_margin() with single ScrollUp call (not loop)
  • early returns for disabled states: Fixed(0) and Proportional(>=1.0)
  • clamps Fixed to terminal height and Proportional to 0.0-1.0 range
  • derives Default trait idiomatically with #[default] attribute
  • adds 4 unit tests for config parsing
  • adds 8 PTY integration tests for cursor positioning verification
  • updates JSON schema and snapshots
[editor]
# Proportional: reserve bottom fraction (0.0 = top, 1.0 = disabled)
bottom_margin = { proportional = 0.5 }
# Fixed: reserve exact line count
bottom_margin = { fixed = 10 }
# Disabled (default): no margin
bottom_margin = "disabled"

Implements #114

@eitsupi
Copy link
Copy Markdown
Owner

eitsupi commented Mar 12, 2026

Thank you for your contribution!

I did some research and it seems that if we implement such a function independently in arf, it may cause inconsistencies with the internal state of reedline.
I think we should add a new feature to reedline. What do you think?

@brancengregory
Copy link
Copy Markdown
Author

Seems reasonable to me! I'll take a look. Thanks for your work on this, huge fan!

@eitsupi
Copy link
Copy Markdown
Owner

eitsupi commented Mar 12, 2026

Thanks!

It might be a good idea to add an argument to Painter::initialize_prompt_position.
It already calls cursor::position() and sets prompt_start_row, so adding a margin check there would keep everything consistent with reedline's internal state.

@brancengregory
Copy link
Copy Markdown
Author

So I tried implementing the bottom margin in reedline here per your suggestion about Painter::initialize_prompt_position. I used a field on Painter rather than a separate argument (small implementation difference, same concept).
It works for initial placement, but reedline ends up hopping around and erasing R output since it isn't aware of when R prints.

I'm not seeing another approach beyond the current one that maintains reedline's state consistency while handling external output. Do you see something I'm missing?

@brancengregory
Copy link
Copy Markdown
Author

Sidenote, I think bottom_buffer_fraction would be better named bottom_margin with typed config for clarity. It would result in a config option in this format:

[prompt]
bottom_margin = { type = "proportional", value = 0.5 }

or

[prompt]
bottom_margin = { type = "fixed", value = 10 }

@eitsupi
Copy link
Copy Markdown
Owner

eitsupi commented Mar 12, 2026

Thanks for trying the reedline approach and sharing the results!

I looked into Painter internals and I agree repaint_buffer has its own scrolling logic and is_reset() checks that conflict with scrolling in initialize_prompt_position.
Doing it outside reedline before read_line() seems more practical.

That said, I have some feedback on the current implementation:

  • ScrollUp(1) in a loop should be ScrollUp(lines_to_scroll)
  • fraction <= 0.0 early-returns (disables the feature), but the doc says 0.0 means "top of screen". These are contradictory.
  • cursor::position() involves terminal I/O (query + response). Calling it every REPL iteration could be noticeable.
  • [prompt] section is for format strings and indicators. Screen positioning feels like it belongs in [editor] or a new section.
  • No tests for the scroll behavior. This project has PTY-based integration tests (using vt100 + portable-pty) that can verify cursor positioning and screen state, so a test like that would be needed here. Let me know if that's difficult to add, I can help.

- adds BottomMargin enum with Fixed, Proportional, and Disabled variants
- places config in [editor] section for terminal positioning control
- implements ensure_bottom_margin() with single ScrollUp call
- early returns for disabled states: Fixed(0) and Proportional(>=1.0)
- clamps Fixed to terminal height and Proportional to 0.0-1.0 range
- derives Default trait idiomatically with #[default] attribute
- adds 4 tests for parsing Fixed, Proportional, Disabled, and default
- updates JSON schema and snapshots

TOML configuration:
  bottom_margin = { fixed = 10 }          # Reserve 10 lines at bottom
  bottom_margin = { proportional = 0.5 }  # Reserve bottom 50%
  bottom_margin = "disabled"            # No margin (default)
- test proportional margin keeps prompt in upper half (0.5)
- test fixed margin reserves exact line count (5 lines)
- test disabled margin allows prompt at bottom
- test proportional = 0.0 pins prompt to top
- test fixed = 0 behaves like disabled (zero-cost)
- test large fixed values clamp to terminal height
- test high proportional values (0.95) keep prompt near top
- test window resize handling and margin consistency
@brancengregory brancengregory changed the title feat: add bottom_buffer_fraction config for prompt positioning feat: add bottom_margin config for prompt positioning Mar 12, 2026
@brancengregory
Copy link
Copy Markdown
Author

Okay I made those changes, cleaned up the config format a bit, and attempted to do some PTY tests. Also updated the PR title and initial description comment

@brancengregory
Copy link
Copy Markdown
Author

The only thing I didn't address is this point:

cursor::position() involves terminal I/O (query + response). Calling it every REPL iteration could be noticeable.

That is still the case but I think this is an inherent tradeoff if we don't want the prompt line to drift at all. If some drift were tolerable then I guess some kind of 'every X iteration' would be possible but I don't think that would be the ideal UX. I personally think the 2-5ms per prompt is acceptable as long as it stays an opt-in feature.

Copy link
Copy Markdown
Owner

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I'll take a look when I have time.

For now, I'm curious about the format of the configuration file.

Comment thread artifacts/arf.schema.json
Comment on lines +182 to +184
"enum": [
"disabled"
]
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the world of TOML, where there are no nulls, I think not setting anything represents null.

I think that instead of setting a special value "disabled" here, the default should be to not display the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants