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

about_popup: Terminal size added. #1542

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

Swarnim114
Copy link
Collaborator

closes #1536

What does this PR do, and why?

Adds a Terminal size label under the "Detected Environment " in About popup

image

External discussion & connections

How did you test this?

  • Manually - Behavioral changes
  • Manually - Visual changes
  • Adapting existing automated tests
  • Adding automated tests for new behavior (or missing tests)
  • Existing automated tests should already cover this (only a refactor of tested code)

Self-review checklist for each commit

  • It is a minimal coherent idea
  • It has a commit summary following the documented style (title & body)
  • It has a commit summary describing the motivation and reasoning for the change
  • It individually passes linting and tests
  • It contains test additions for any new behavior
  • It flows clearly from a previous branch commit, and/or prepares for the next commit

Visual changes

image

@zulipbot zulipbot added the size: S [Automatic label added by zulipbot] label Aug 3, 2024
@Swarnim114 Swarnim114 force-pushed the main branch 2 times, most recently from 03845de to 2ce2242 Compare August 3, 2024 11:43
@zulipbot
Copy link
Member

zulipbot commented Aug 3, 2024

Hello @Swarnim114, it seems like you have referenced #1536 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #1536..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

@zulipbot
Copy link
Member

zulipbot commented Aug 3, 2024

ERROR: Label "Pr need review" does not exist and was thus not added to this pull request.

@Swarnim114
Copy link
Collaborator Author

@zulipbot label "PR needs review"

@zulipbot zulipbot added the PR needs review PR requires feedback to proceed label Aug 3, 2024
@Swarnim114
Copy link
Collaborator Author

Swarnim114 commented Aug 3, 2024

@rsashank, I apologize for the confusion. This will be my final pull request for this issue. Thank you for your patience.This is the same code from before,

@@ -301,6 +302,9 @@ def popup_with_message(self, text: str, width: int) -> None:
self.show_pop_up(NoticeView(self, text, width, "NOTICE"), "area:error")

def show_about(self) -> None:
terminal_size = shutil.get_terminal_size()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You changed to using shutil here - was there as a specific reason for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Swarnim114 To confirm, this looks good, but I wasn't sure why you switched to this.

Copy link
Collaborator Author

@Swarnim114 Swarnim114 Aug 21, 2024

Choose a reason for hiding this comment

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

@neiljp I thought it would be just simpler and give me no issues..

Copy link

Choose a reason for hiding this comment

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

os.get_terminal_size() works fine, no?
I tried to replicate what @Swarnim114 did in this PR but without using shutil and it's working for me.

Screenshot 2024-10-29 115454

@Swarnim114 are there any shortcomings of doing it this way?

Copy link

Choose a reason for hiding this comment

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

@neiljp, any advice on this?

@neiljp neiljp added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs review PR requires feedback to proceed labels Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback size: S [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add current terminal window size to About popup
4 participants