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

Abstraction for Cursor Movements (Issue #46) #48

Merged
merged 3 commits into from Apr 26, 2020
Merged

Abstraction for Cursor Movements (Issue #46) #48

merged 3 commits into from Apr 26, 2020

Conversation

felixguendling
Copy link
Contributor

Try to address #46 in a PoC.

@felixguendling
Copy link
Contributor Author

felixguendling commented Apr 24, 2020

I was experimenting with the library and it did not work well on Windows with multi-progress bar.
This is essentially the fix from @wolfv which seems to work well on Windows.

Regarding Codacy: I think this is a false positive. It is just a nested preprocessor #if.

Edit: I only adjusted the multi_progress_bar.cpp example but I can adjust the other examples too, if there is any interest in this PR. Otherwise, please close.

Edit 1: There are more changes because my editor automatically applies clang-format. But since there is a .clang-format file in the repository root, I assume that the files should be formatted with clang-format anyway?

@p-ranav
Copy link
Owner

p-ranav commented Apr 24, 2020

Hello, I'm interested in this PR.

Can you explain the semantics of these two statements?

  indicators::enable_cursor_movement();
  indicators::show_console_cursor(false);

If I was a user, why would I need to make these calls? What happens if I don't make these calls?

@wolfv
Copy link
Contributor

wolfv commented Apr 25, 2020

For our use case we ended up just calling these lines to enable VT100 escape codes on Windows:

https://github.com/QuantStack/mamba/blob/100f4de8580aab00cc9adb061949e607339ebaa0/src/output.cpp#L241-L248

You might also be interested in these to get the width of the terminal on Win & Unix:

https://github.com/QuantStack/mamba/blob/100f4de8580aab00cc9adb061949e607339ebaa0/src/output.cpp#L40-L54

And here are the cursor movements:

https://github.com/QuantStack/mamba/blob/100f4de8580aab00cc9adb061949e607339ebaa0/include/output.hpp#L17-L101

Note: since we could enable ANSI / VT100 escpae codes on Win (works from Win 10) we didn't bother with using the Win APIs for cursor movement.
I guess if you want to support older Win versions that's necessary.

@felixguendling
Copy link
Contributor Author

felixguendling commented Apr 25, 2020

If I was a user, why would I need to make these calls? What happens if I don't make these calls?

I thought that the show_console_cursor(false) call was a more comprehensible way than std::cout << "\e[?25l"; which is used elsewhere. I hoped that the naming was clear: show_console_cursor(false) hides the cursor, show_console_cursor(true) makes it visible. If you do not make this call, you have some flicker because the cursor jumps between the lines. It's not absolutely necessary but I find the jumping cursor a bit annoying.

enable_cursor_movement() is not required if we use the Windows SetConsoleCursorPosition API. I removed it. If supporting older versions of Windows is not a goal, it is probably easier to enable output processing on Windows and not use the SetConsoleCursorPosition function. The current implementation in the PR targets older Windows versions, too.

@p-ranav
Copy link
Owner

p-ranav commented Apr 25, 2020

@felixguendling Do you mind updating the other samples too (and probably the README)? We can merge this.

@felixguendling
Copy link
Contributor Author

felixguendling commented Apr 26, 2020

Now, the string "\e[?25" is only used in the show_console_cursor() function. Every example/demo (including the README) uses this function now and should therefore work on Windows as well.

@p-ranav p-ranav merged commit 3d1d01b into p-ranav:master Apr 26, 2020
@felixguendling felixguendling deleted the win-cursor-mv branch April 26, 2020 17:30
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.

None yet

3 participants