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.

Sign up for GitHub

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

Jump to bottom

[GUI] Add context manager support for ui.Gui #3055

Merged
merged 8 commits into from
Oct 8, 2021
Merged

[GUI] Add context manager support for ui.Gui #3055

merged 8 commits into from
Oct 8, 2021

Conversation

Xuanwo
Copy link
Contributor

@Xuanwo Xuanwo commented Sep 30, 2021 •
edited
Loading

Signed-off-by: Xuanwo github@xuanwo.io

Related issue = close #3015

Note

This PR will introduce a new API called sub_window.

The function name is chosen without much deep thought. Please feel free to propose other options.

@Xuanwo
[python] Add context manager support for ui.Gui
4627711 
Signed-off-by: Xuanwo <github@xuanwo.io>
@CLAassistant
Copy link

CLAassistant commented Sep 30, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Sep 30, 2021 •
edited
Loading

✔️ Deploy Preview for jovial-fermat-aa59dc canceled.

🔨 Explore the source changes: a772cc0

🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-fermat-aa59dc/deploys/615fcdc115a2da0008ea46ea

@Zheaoli
Copy link
Contributor

Zheaoli commented Sep 30, 2021

LGTM , would you mind adding some extra unit tests for the change?

@Xuanwo
Copy link
Contributor Author

Xuanwo commented Sep 30, 2021

LGTM , would you mind adding some extra unit tests for the change?

Of course. Can you guide me to the existing unit tests about the GUI class?

yihong0618
yihong0618 reviewed Sep 30, 2021
View reviewed changes
Copy link
Collaborator

@yihong0618 yihong0618 left a comment

Choose a reason for hiding this comment

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

def __exit__(self, exc_type, exc_val, exc_tb):

use may better

def __exit__(self, *args)

python/taichi/ui/gui.py Outdated Show resolved Hide resolved
@Xuanwo
Add args for __exit__
766d9a8 
Signed-off-by: Xuanwo <github@xuanwo.io>
Zheaoli
Zheaoli approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

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

LGTM

python/taichi/ui/gui.py Outdated Show resolved Hide resolved
@Zheaoli
Copy link
Contributor

Zheaoli commented Sep 30, 2021

PTAL @ailzhang

frostming
frostming reviewed Sep 30, 2021
View reviewed changes
python/taichi/ui/gui.py Outdated Show resolved Hide resolved
@Xuanwo
Fix the wrong usage of __enter__
284834c 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Copy link
Contributor Author

Xuanwo commented Sep 30, 2021

This PR will introduce a new API called subwindow. The function name is chosen without much deep thought. Please feel free to propose other options.

Zheaoli
Zheaoli reviewed Sep 30, 2021
View reviewed changes
python/taichi/ui/gui.py Outdated Show resolved Hide resolved
k-ye
k-ye reviewed Sep 30, 2021
View reviewed changes
python/taichi/ui/gui.py Outdated Show resolved Hide resolved
@Xuanwo
Rename to sub_window
22f0241 
Signed-off-by: Xuanwo <github@xuanwo.io>
ailzhang
ailzhang reviewed Sep 30, 2021
View reviewed changes
python/taichi/ui/gui.py Outdated Show resolved Hide resolved
@Xuanwo
Update comments
d9572c3 
Signed-off-by: Xuanwo <github@xuanwo.io>
@AmesingFlank
Copy link
Collaborator

LGTM , would you mind adding some extra unit tests for the change?

Of course. Can you guide me to the existing unit tests about the GUI class?

Unfortunately, we don't have any tests for GGUI yet, because we haven't figured out headlessly running GGUI. Don't worry about writing tests for now.

I will work on making GGUI testable when I have time.

@Xuanwo
Update comments for sub_window
b78706c 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo mentioned this pull request Sep 30, 2021
Open
ailzhang
ailzhang approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@ailzhang ailzhang left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for contributing!

python/taichi/ui/gui.py
"""Creating a context manager for subwindow

Note::
All args of this method should align with `begin`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this line should be removed along with begin/end in the next PR ;)

@ailzhang ailzhang changed the title [python] Add context manager support for ui.Gui [GUI] Add context manager support for ui.Gui Sep 30, 2021
@Xuanwo
Fix comment style
8e0b5cb 
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo
Copy link
Contributor Author

Xuanwo commented Oct 8, 2021

Hello, is there any thing I can do to help this PR get merged?

k-ye
k-ye approved these changes Oct 8, 2021
View reviewed changes
Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@strongoier
Copy link
Contributor

Hello, is there any thing I can do to help this PR get merged?

You need to pass the code format check below :-) The easiest way is to leave a comment /format here to let the robot help format your code. You may also want to read our full contributor guide (https://docs.taichi.graphics/lang/articles/contribution/contributor_guide) for more details.

@Xuanwo
Copy link
Contributor Author

Xuanwo commented Oct 8, 2021

/format

@Xuanwo
Copy link
Contributor Author

Xuanwo commented Oct 8, 2021

Merge with master to fix the code format check.

@strongoier strongoier merged commit 4908d5b into taichi-dev:master Oct 8, 2021
@Xuanwo Xuanwo deleted the window-gui-context branch October 12, 2021 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frostming frostming frostming left review comments

@yihong0618 yihong0618 yihong0618 left review comments

@Zheaoli Zheaoli Zheaoli approved these changes

@k-ye k-ye k-ye approved these changes

@ailzhang ailzhang ailzhang approved these changes

@AmesingFlank AmesingFlank Awaiting requested review from AmesingFlank

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use python's with construct for IMGUI in GGUI
9 participants
@Xuanwo @CLAassistant @Zheaoli @AmesingFlank @strongoier @ailzhang @k-ye @yihong0618 @frostming