-
-
Notifications
You must be signed in to change notification settings - Fork 270
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
Conversation
39afbb2
to
ec442aa
Compare
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.
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>| |
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 could benefit from adding a tiny bit of description clarifying what 'information'.
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.
application information?
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 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", |
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 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.
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 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.
@rsashank Some quick notes based on the content of the PR right now:
|
765ac1c
to
ec9c5ae
Compare
This lgtm! |
Tests updated. Fixes zulip#1493.
@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. |
What does this PR do, and why?
Adds hotkey 'c' to copy information from the about popup. It's copied in this format:
External discussion & connections
Copy Information from AboutPopup #T1493
How did you test this?
Self-review checklist for each commit
Visual changes