Skip to content

Conversation

@fastrgv
Copy link
Contributor

@fastrgv fastrgv commented Oct 19, 2023

For several of my apps I needed the window height. This change has been tested on OSX, Windows, and several flavors of linux.
See, for example, "RetroArcade". This is one of my projects on GitHub.

@CLAassistant
Copy link

CLAassistant commented Oct 19, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@setton setton 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 very much for this contribution. I have left a few comments - only stylistic, the code itself looks good to me. Let us know if you're happy to make those changes.

src/terminals.c Outdated
CONSOLE_SCREEN_BUFFER_INFO csbiInfo;
if (GetConsoleScreenBufferInfo (handle, &csbiInfo)) {
return (int)csbiInfo.dwSize.X;
//return (int)csbiInfo.dwSize.X; // buffer width
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the commented-out line. (Same comment on line 148)

src/terminals.c Outdated
return (int)csbiInfo.dwSize.X;
//return (int)csbiInfo.dwSize.X; // buffer width
return (int)(csbiInfo.srWindow.Right
-csbiInfo.srWindow.Left + 1); // window width
Copy link
Member

Choose a reason for hiding this comment

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

The indentation of this line is off - please remove the tabs (same comment for line 150)

-- Whether the associated terminal is stdout (windows only)
end record;


Copy link
Member

Choose a reason for hiding this comment

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

Please remove these extra lines.

-- Return the height of the terminal, or -1 if that height is either
-- unknown or does not apply (as is the case for files for instance).


Copy link
Member

Choose a reason for hiding this comment

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

Please remove one of these blank lines.

return Internal (Boolean'Pos (Self.FD = Stderr));
end if;
end Get_Width;

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the extra blank lines - there should be at most one blank line as separation. (Same goes for lines 452-453.)

Copy link
Collaborator

@t-14 t-14 left a comment

Choose a reason for hiding this comment

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

As a general comment, can you also add a new test exercising the additional code?

@fastrgv
Copy link
Contributor Author

fastrgv commented Oct 20, 2023

Ok, I think all suggested changes have been made. And I attached a test driver in this message.

testwin_adb.txt

@t-14
Copy link
Collaborator

t-14 commented Oct 21, 2023

And I attached a test driver in this message.

Thanks, but this needs to be added as a new test case in testsuite/tests/terminal.

Test of new get_lines function.
@fastrgv
Copy link
Contributor Author

fastrgv commented Oct 21, 2023

Ok. I just added it to ~testsuite/tests/terminal/testlines.adb

adacore-bot pushed a commit that referenced this pull request Mar 25, 2024
Provide a Get_Lines function in GNATCOLL.Terminal.

Github PR: #81
adacore-bot pushed a commit that referenced this pull request Mar 25, 2024
Merge #81

See merge request eng/toolchain/gnatcoll-core!86
@Jicquel
Copy link
Collaborator

Jicquel commented Mar 25, 2024

Hello @fastrgv, thank you for your contribution! Your commits have been squashed and merged here: 724c8db.

@Jicquel Jicquel closed this Mar 25, 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.

5 participants