-
Notifications
You must be signed in to change notification settings - Fork 24
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
added uiEntry + fixed a wrong function call #27
Conversation
""" | ||
Sets the text of the entry. | ||
|
||
:param text: the text of the entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about making this just string
instead of the text of the entry
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right
|
||
def getReadOnly(self): | ||
""" | ||
Sets whether the entry is read only or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Gets...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or Returns 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops
:return: string | ||
""" | ||
|
||
return bool(clibui.uiEntryReadOnly(entry)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cast should be in wrapper I think, and return value should be int instead of string, in docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right. The reason I did not do it is simply because I wrote the code before the issue was submitted. (24 Jun)
I can fix it on this PR; however I already did another branch -which I will submit in a PR after this one- which removes all the casts in pylibui/libui/*.py
files. It's which one you prefer :-)
Concerning the "int" instead of "string", you are right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It's okay, we can merge this one, then send the others 👍
Sets whether the entry is read only or not. | ||
|
||
:param entry: uiEntry | ||
:param read_only: bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right
super().setUp() | ||
self.entry = Entry() | ||
self.password_entry = PasswordEntry() | ||
self.search_entry = SearchEntry() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you used password_entry
and search_entry
in this file, am I overlooking them? If you didn't use I think we can remove them because they all share the same functions, am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I do not need these two variables. I just instanced them here at first, but don't need it.
self.assertEqual(self.entry.getReadOnly(), False) | ||
|
||
def test_onChanged_is_called(self): | ||
raise # TODO: how do we simulate mouse press ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we will remove all lines like this, why did you add one here, maybe a mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as comment above, because I wrote the code before. I already did another commit to remove these raises (https://github.com/superzazu/pylibui/commit/4a4f7f6e66db6506a9eef020a372e3595ed61eb5)
Okay, thanks for the feedback (and sorry for the typos...), I'm going to do another commit :-) As I said, concerning the two comments about" removing the raise in tests" + "putting the casts in controls"; it is because this code is a bit old. |
No, it will be better if they come in another PR. Sorry for comments, I haven't fully understood why you're mentioning about other two PRs in this one, now it's clear 👍 |
BTW, what is your output after executing all test suite? It seems you've fixed the segmentation thing in checkbox, for me. |
On my mac, I never had any problems with segfaults while running tests (only when running examples). Here's the complete output:
|
Erm, I'm dumb. :D Of course I did something to the checkboxes : f60452d I was calling a wrong method (uiButtonOnClicked instead of uiCheckboxOnToggled). That may be the problem you were having while running tests. I commited the fixes to these typos. |
Weird. This one is just testing control, FYI. And I still have an segfault about while testing window.
|
I saw your change and I'm now suspecting about another wrong call in window. |
Weird. I don't have this problem here:
I checked window files, I did not see anything obvious like the "wrong call" I just fixed. EDIT: I noticed there is no "bool" cast in "uiControlVisible" nor in "Control.getVisible()". Maybe is it the source of the problem ? It's weird... |
Let's focus on this PR and merge it, I will try to find the problem soon. |
password_entry = PasswordEntry() | ||
|
||
vbox = VerticalBox() | ||
vbox.setPadded(10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using 10 here? I thought it expects a bool value here, am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, is it a mistake. I have to correct this
By the way, I think we should replace the occurrences of ".setMargined(1)" / ".setPadded(1)" by something like ".setMargined(True)" (in the examples). I'll open an issue for that.
Thanks for PR |
See, you have another 10 on the line window.setMargined, please fix it as 22 Eki 2016 19:07 tarihinde "Nicolas Allemand" [email protected]
|
The |
Sorry for that. I've sent that comment four days ago via Gmail. It took a bit late to appear here 😑 I was talking about line 30 in entry example. |
I added uiEntries + fixed a wrong function call in
pylibui/libui/checkbox.py
In the file
examples/entry.py
, I temporarily commentedapp.close()
to avoid crashes (and added a comment that links to the issue #18)Note: the deletion of "raise" in tests is already done in another branch (here). I'll make more PR (this one among two others which fixes #23 & #24) when this one is merged
What do you guys think ?