Skip to content

Conversation

rivy
Copy link
Contributor

@rivy rivy commented May 17, 2024

Fixes #23872.

@rivy rivy force-pushed the fix.consolesize-winos branch from dde9496 to 39bc84a Compare May 17, 2024 21:11
@rivy
Copy link
Contributor Author

rivy commented May 17, 2024

I haven't implemented an underflow check for the subtractions, but I can if requested.

@rivy
Copy link
Contributor Author

rivy commented May 21, 2024

I read the docs a bit closer and decided that over/under-flow protection was warranted given the vagueness.
Return will be either correct, or, if bad values are received, { cols: 0, rows: 0 }.

@bartlomieju bartlomieju requested a review from littledivy May 22, 2024 14:23
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

Looks good, one comment

@bartlomieju
Copy link
Member

@littledivy can you, please, shepherd this PR to landing?

@bartlomieju bartlomieju added this to the 1.46 milestone Jul 18, 2024
@littledivy littledivy changed the base branch from v1.43 to main July 18, 2024 03:18
@littledivy littledivy changed the base branch from main to v1.43 July 18, 2024 03:18
@dsherret dsherret changed the base branch from v1.43 to main August 2, 2024 13:24
@dsherret dsherret changed the title fix(runtime/windows): (WinOS) Fix calculation of console size fix(runtime/windows): fix calculation of console size Aug 2, 2024
@dsherret
Copy link
Member

dsherret commented Aug 2, 2024

@rivy can you rebase this PR onto main? Also, please allow maintainers the ability to push to your branches in the future. I went through and fixed this PR myself and then wasn't able to push.

@rivy
Copy link
Contributor Author

rivy commented Aug 3, 2024

@rivy can you rebase this PR onto main? Also, please allow maintainers the ability to push to your branches in the future. I went through and fixed this PR myself and then wasn't able to push.

Happy to rebase.

I didn't specifically disallow maintainer edits; I just opened a quick PR.
But I also don't now see the "Allow edits from maintainers" checkbox to enable access.
I'm not sure why it's not there... some permission issue? on my fork? Any idea?

@rivy rivy force-pushed the fix.consolesize-winos branch from 9f50e84 to cf57755 Compare August 3, 2024 14:51
@rivy
Copy link
Contributor Author

rivy commented Aug 3, 2024

@dsherret

Also, please allow maintainers the ability to push to your branches in the future. I went through and fixed this PR myself and then wasn't able to push.

I did some quick research, and the permission problem might be related to my fork being "organization owned" (I segregate forks which I don't "own" but commit to into a "rivy-fix" organization). It looks like cross-organization edit permission is not enabled. There's some discussion about this as a problem in the community discussions (see https://github.com/orgs/community/discussions/5634).

Ultimately, it looks like the missing permission is at least a documentation problem if not a frankly ignored bug.

ref: https://github.com/orgs/community/discussions/5634
ref: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/about-permissions-and-visibility-of-forks

@dsherret dsherret merged commit a9cdfdc into denoland:main Aug 12, 2024
17 checks passed
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.

🐛(WinOS) Deno.consoleSize() incorrectly returns console buffer size
4 participants