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

Save ('s') and restore ('u') support added #137

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kgeorgiy
Copy link

Includes test and demo plus minor refactoring of ansitowin32

Georgiy Korneev added 5 commits July 13, 2017 18:05
Existing 'get_position' method is flawed in the following ways:
 * Naming: its setter is 'set_cursor_position', not 'set_position'
 * Abstraction leak: returns internal Win32 structure
 * There is no way to get adjusted cursor position
 * Receives handle, instead of on_stderr flag
There are no more uses of get_position in the code base, so it could be deprecated, and, possibly, removed in future
Copy link
Collaborator

@wiggin15 wiggin15 left a comment

Choose a reason for hiding this comment

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

Generally, this looks good. Please see my review comments

@@ -89,31 +91,38 @@ def get_position(self, handle):
position.Y += 1
return position

def get_cursor_position(self, on_stderr=False, adjust=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if we need this function. We already have get_position which returns the cursor position (we can rename that function to make it clear it's about the cursor). Regarding the adjustments, we have a code for adjusting the cursor position in SetConsoleCursorPosition. We can play with its adjust parameter based on if it's needed or not when restoring with 'u', instead of having more code that deals with this adjustments.

Copy link
Owner

@tartley tartley Oct 7, 2021

Choose a reason for hiding this comment

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

I'm sensitive to wiggin's criticism here which I think might be valid, but am enthusiastic about the PR overall. Assuming @kgeorgiy is not still around / motivated after 4 years, I'll take a closer look at this code tonight, and try to merge it...

Apologies for resurrecting this after ignoring it for years!

handle = win32.STDERR
position = self.get_position(handle)
adjusted_position = (position.Y + y, position.X + x)
(cy, cx) = self.get_cursor_position(on_stderr, adjust=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we decide to continue to use get_position instead of the new get_cursor_position, we will not need this change.

@tartley
Copy link
Owner

tartley commented Oct 13, 2020

Hey. FYI, yesterday I created a PR to test releases before we push them to PyPI. When that is merged, I'll be more confident about resuming merges and releases. I'll try to look at this PR soon. Thank you for creating it!

win32.SetConsoleTextAttribute(handle, attrs)

@staticmethod
def get_handle(on_stderr=False):
return win32.STDERR if on_stderr else win32.STDOUT
Copy link
Owner

Choose a reason for hiding this comment

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

I like this ^

@tartley tartley added the 0.4.5 Get merged for 0.4.5 release label Oct 7, 2021
@tartley tartley added 0.4.6 Get merged for 0.4.6. release and removed 0.4.5 Get merged for 0.4.5 release labels Jun 14, 2022
@crackwitz
Copy link

I could have used this feature right now. Anyone with privileges, please take another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.4.6 Get merged for 0.4.6. release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants