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

Functionality to copy information from the about popup #1521

Merged
merged 4 commits into from
Jun 22, 2024

Conversation

rsashank
Copy link
Member

@rsashank rsashank commented Jun 19, 2024

What does this PR do, and why?

Adds hotkey 'c' to copy information from the about popup. It's copied in this format:

#### Application
Zulip Terminal: 0.7.0+git

#### Server
Version: 9.0-dev-3226-gbc50a9743b
Feature level: 265

#### Application Configuration
Theme: zt_dark
Autohide: disabled
Maximum footlinks: 3
Color depth: 256
Notifications: disabled
Exit confirmation: enabled

#### Detected Environment
Platform: WSL
Python: 3.10.12 (CPython) 

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

Before After
image image

@zulipbot zulipbot added size: M [Automatic label added by zulipbot] area: popup: about enhancement New feature or request good first issue Good for newcomers labels Jun 19, 2024
@rsashank rsashank force-pushed the copyaboutpopup branch 3 times, most recently from 39afbb2 to ec442aa Compare June 19, 2024 15:59
Copy link
Collaborator

@Niloth-p Niloth-p left a comment

Choose a reason for hiding this comment

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

Good PR, Sashank! This should really help issue reporters.
It works fine.

Clarifying what 'information' would help not only in the help texts, but also in the commit messages / descriptions.

Hard coding the string does not seem sustainable. It's preferable to just generate the string like this.

sections = []
for section_title, properties in contents:
	formatted_properties = '\n'.join(f"{prop}: {value}" for prop, value in properties)
	sections.append(f"{section_title}\n{formatted_properties}")
self.copy_info = '\n\n'.join(sections)

about_keys = ", ".join(map(repr, display_keys_for_command("COPY_ABOUT_INFO")))
contents.append((" ", [(f"Press {about_keys} to copy this information.", "")]))

Add the keys to the contents after extracting the string version.

I'm wondering if we're going to be adding enough copy commands to create a help category just for them.

docs/hotkeys.md Outdated
@@ -10,6 +10,7 @@
|Show/hide about menu|<kbd>Meta</kbd> + <kbd>?</kbd>|
|Go Back|<kbd>Esc</kbd>|
|Open draft message saved in this session|<kbd>d</kbd>|
|Copy information from the About popup|<kbd>c</kbd>|
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could benefit from adding a tiny bit of description clarifying what 'information'.

Copy link
Member Author

Choose a reason for hiding this comment

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

application information?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I adjusted this to include About Menu, and specifically to the clipboard; it's also not just regarding the application, so it seems fine to be this broad.

@@ -261,7 +261,8 @@ def test_categories(self) -> None:
"Application",
"Server",
"Application Configuration",
"Detected environment",
"Detected Environment",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This case fix is not related to the commit message.
Not sure how we're dealing with these, but even if we're including it in this commit, I assume we'd need to add a mention in the commit description.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I split this out in my push just now - it's generally just easier to track what's happening in the git log if each commit makes a single change.

@Niloth-p Niloth-p added PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback and removed PR needs buddy review labels Jun 20, 2024
@neiljp
Copy link
Collaborator

neiljp commented Jun 20, 2024

@rsashank Some quick notes based on the content of the PR right now:

  • Re an 'empty category', I was thinking more along the lines of a category having no contents, but the category title being the copy hint, making the text stand out like the other titles
  • To style the help key similar to elsewhere in the UI, we want it to have [] around the key, and be to the right of the text
  • Re the copied content, having it suitable for pasting into an issue would be preferable, so using some (sub)title markdown for each category title would look good, and also help them stand out in plain text form
  • I support a flexible implementation closer to @Niloth-p's suggestion re the formatting, since the contents of the popup may change over time and a more algorithmic approach will automatically stay up to date (though the tests should be more static, to track behavioral changes)

@rsashank rsashank force-pushed the copyaboutpopup branch 2 times, most recently from 765ac1c to ec9c5ae Compare June 20, 2024 12:13
@rsashank
Copy link
Member Author

Thanks for the review @Niloth-p and @neiljp! 😄

I've updated the PR based on the feedback, thanks for providing the snippet @Niloth-p :)

  • Markdown Preview

image

  • This is how the popup looks now

image

@rsashank rsashank added PR needs buddy review PR needs mentor review and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 20, 2024
@Niloth-p
Copy link
Collaborator

Niloth-p commented Jun 21, 2024

This lgtm!
The feedback has been integrated into the code.
And the PR description is quite helpful and accurate.

@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Jun 22, 2024
@neiljp
Copy link
Collaborator

neiljp commented Jun 22, 2024

@rsashank Thanks for this update 👍

Since this looked very close and you're due to be away for a few days, this seemed close to merge with some minor changes - and opens up editing the issue templates to reference this feature!

I made some minor text updates, and kept the change of case in an earlier commit to keep it clearly separate. I realized before pushing back here that you didn't have a test for the copied text output, so added a commit with that myself.

@neiljp neiljp merged commit f61a435 into zulip:main Jun 22, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: popup: about enhancement New feature or request good first issue Good for newcomers size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve user issue reporting - add copy feature to About popup
4 participants