Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add commands for scrolling region manipulation. #916

Closed
wants to merge 1 commit into from

Conversation

nfachan
Copy link

@nfachan nfachan commented Aug 20, 2024

One command sets the scrolling region, the other resets it to be the whole screen.

I don't know what to do about the pre-windows-10 winapi.

Is this something that can be implemented using the winapi? If not, what happens then? I'd love some guidance.

One command sets the scrolling region, the other resets it to be the
whole screen.
@nfachan nfachan requested a review from TimonPost as a code owner August 20, 2024 21:40
Copy link
Collaborator

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Regarding the winapi side of this, there doesn't seem like there's an exact equivalent of this as a command in winapi. The desired behavior of scrolling a region can be done in a single call instead of one for setting the region and one to scroll. (see https://learn.microsoft.com/en-us/windows/console/scrolling-the-screen-buffer).

It might be possible to emulate this behavior, but it would be complex, likely have many edge cases and hence would probably cause a significant maintenance burden to get right. I wouldn't recommend doing so.

The better approach is to return an io error indicating that the feature is unsupported as this allows apps to gracefully do something else.


#[cfg(windows)]
fn execute_winapi(&self) -> io::Result<()> {
unimplemented!("not implemented for winapi");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Other methods that are unsupported instead return and io::Error with the unsupported error kind and a nice error message. I'd recommend doing the same here and below.

@nfachan
Copy link
Author

nfachan commented Aug 21, 2024

The calling application is setting the region, scrolling, then resetting the region. So it probably makes sense to just change the commands to be something like ScrollRegionUp and ScrollRegionDown.

I'll attempt to make that change.

Thanks for the pointer to the Windows API documentation.

@joshka
Copy link
Collaborator

joshka commented Aug 21, 2024

The calling application is setting the region, scrolling, then resetting the region. So it probably makes sense to just change the commands to be something like ScrollRegionUp and ScrollRegionDown.

I'll attempt to make that change.

Thanks for the pointer to the Windows API documentation.

I think what you have here is reasonable - mainly as it maps directly to an actual command that would be sent. Abstracting the region stuff might still be reasonable to do (I think it's probably not), but should be implemented as some sort of composite of the lower level commands you have here.

Putting my Ratatui hat on, having a scroll area work like this this might be really nice to experiment with.

@nfachan nfachan closed this Aug 24, 2024
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