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

change: Add support for more ncurses functions #5

Merged
merged 6 commits into from
Aug 31, 2018

Conversation

geolessel
Copy link
Contributor

Thanks for the project! I've been using it for a side project while I'm learning Crystal and have found it pretty easy to use. However, there were some ncurses functions that I wanted to have supported and added them here. Do these look OK?

I am learning Crystal and ncurses at the same time, so feel free to point me in a better direction if needed.

Copy link
Owner

@agatan agatan left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!! It's a really great work to add lacking features of original ncurses.

I reviewed your PR and added some comments. Please let me know your opinion.

@@ -23,6 +24,26 @@ module NCurses
end
end

def self.subwin(height, width, y, x, parent, &blk)
win = new(LibNCurses.subwin(parent, height, width, y, x))
wind.closed = false
Copy link
Owner

Choose a reason for hiding this comment

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

win.closed = false ?
@closed is false by default, so I guess this line is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agatan yes, I don't believe it is necessary.

@@ -3,6 +3,7 @@ require "./raw_window"

module NCurses
class Window
property closed : Bool = false
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of this line?
I think that closed is a private (internal) property or read-only property.
If closed property is assigned true by external code by mistake, the window resource will never be closed.

Would you give us any examples to use closed property?

I agree to add closed? method to read @closed state!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agatan It appears I never actually used that closed property anyway, so I'll remove this addition.

@geolessel
Copy link
Contributor Author

@agatan I've updated the PR based on your comments.

Copy link
Owner

@agatan agatan left a comment

Choose a reason for hiding this comment

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

Nice 😄
Thank you for your contribution 🎉

@agatan agatan merged commit 06a4785 into agatan:master Aug 31, 2018
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

2 participants