Copy traceback from the Exception popup

[rsashank]

Allow copying traceback from the Exception has occurred popup.

External discussion & connections

• Discussed in #zulip-terminal in View object has no attribute topic_w
• Fully fixes #
• Partially fixes issue Improve user issue reporting - add copy feature to About popup #1493
• Builds upon previous unmerged work in PR #
• Is a follow-up to work in PR #
• Requires merge of PR #
• Merge will enable work on #

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
Traceback copy feature wasn't available before

[Niloth-p]

It works great! LGTM.

Added a few suggestions for changes.
The naming one though was just me wondering out aloud.

I think you can add a bit more detail to the PR, to help reviewers process it more smoothly. Things like:

• It would be helpful to add a bit more information on what already exists.
  Or just a 'Before' image for the Visual Changes section.
  I got confused on my first glance thinking that you had created the entire popup and all of its content,
  before I realised that this new ExceptionView is an extension of the NoticeView with one extra argument 'traceback' and a keypress override, and the text content of the popup already existed.

• And how one can recreate this popup for testing it (Thanks for sharing that code snippet with me).

• The differences in output between "".join(traceback.format_exception_only(exc[0], exc[1])) and "".join(traceback.format_exception(*exc)) leading to the reason why you named it full_traceback.

[Niloth-p]

This seems a neat addition!
I assume this footer message could be in a separate commit in the PR, for readability, even if it is to be squashed in the merge.

[neiljp]

I'd definitely suggest a distinct way of reporting to the user could be in a separate commit - whether squashed or dropped.

If the intention is to emphasize the error and the available hotkey, then I'd suggest we use a different style in the popup instead.

The footer is generally used for temporary messages, rather than alerting users to hotkeys or events - and we should have the popup to indicate that? Or is this to handle a different case? It would also seem odd to duplicate it, if that is what is happening here?

(In addition, if we intend to use the footer for available hotkeys more broadly, it would seem strange to have this show a hotkey, but only temporarily)

[rsashank]

I put it in a separate commit and took a different approach to setting the footer text. Now, it resets only when exiting the popup. I still included the hint in the popup because the footer reverts to normal after the first copy, making it useful to have it included. What do you think?

[Niloth-p]

Re. the naming,
I know the previous implementation used popup_with_message(), but almost all other methods in core.py seem to be using the show_ prefix, so should we use show_exception_popup() instead?

[neiljp]

Using consistent names for both would be good; a ripe opportunity to refactor first, including possibly renaming the other to be more clear.

[rsashank]

Renamed these, is it fine now?

[neiljp]

@rsashank I'm not sure what status this is in since it doesn't have a label - buddy review, but awaiting update?

I left a few review notes here since I was browsing new PRs and saw this was fairly small and hadn't been reviewed - and is potentially very useful.

I also wanted to note that this does not fix #1493, but rather a related feature, that is certainly still useful 👍 Fixing this and #1493, as well as other issues could motivate refactoring the 'copyable' features into a new popup location, but let's focus on this for now.

---

Briefly re naming the class, while it handles an exception traceback, I wanted to note that this implementation has no specific tie to tracebacks or exceptions - rather it supports the additional behavior of copying a certain string to the clipboard (it need not be called self.traceback or traceback).

Admittedly the naming does currently have a tie to the command-key name you've selected and a descriptive string for reporting the copying status, but they could be passed in as options, or be standardized.

From that perspective, at a minimum this class would be more readable with a slightly different name.

Note that this doesn't mean that the core.py method need be named very generally - callers to it are only concerned about indicating an exception somehow, not the detail that it is a notice with some other copy-able text.

[neiljp]

I'd definitely suggest a distinct way of reporting to the user could be in a separate commit - whether squashed or dropped.

If the intention is to emphasize the error and the available hotkey, then I'd suggest we use a different style in the popup instead.

The footer is generally used for temporary messages, rather than alerting users to hotkeys or events - and we should have the popup to indicate that? Or is this to handle a different case? It would also seem odd to duplicate it, if that is what is happening here?

(In addition, if we intend to use the footer for available hotkeys more broadly, it would seem strange to have this show a hotkey, but only temporarily)

[neiljp]

Using consistent names for both would be good; a ripe opportunity to refactor first, including possibly renaming the other to be more clear.

[rsashank]

Briefly re naming the class, while it handles an exception traceback, I wanted to note that this implementation has no specific tie to tracebacks or exceptions - rather it supports the additional behavior of copying a certain string to the clipboard (it need not be called self.traceback or traceback).

@neiljp But I've inherited the NoticeView class and ensured that this version (ExceptionView) is only used for handling exceptions and copying the traceback. I didn't make any changes to the original class. Given this, should I still consider renaming it? If so, do you have any suggestions for a new name?

[rsashank]

Thanks for the review @neiljp @Niloth-p :)

I've updated the original PR comment to include a table showing the visual changes before and after.

Regarding the code snippet to test this functionality, here's the approach I took (and shared with Niloth):
I went to boxes.py and raised my own ValueError when pressing "Esc" (GO_BACK) from the compose box. Then, I opened the compose box and pressed Esc to trigger the exception.

To test it, add the following code after the elif is_command_key("GO_BACK", key): line (801) in boxes.py:

            import sys
            try:
                raise ValueError("This is a test exception")
            except:
                self.view.controller.raise_exception_in_main_thread(
                    sys.exc_info(), critical=False)

To clarify the difference between the traceback statements:

"".join(traceback.format_exception(*exc)) - complete traceback

  File "/home/ubuntu/zulip-terminal/zulipterminal/ui_tools/boxes.py", line 804, in keypress
    raise ValueError("This is a test exception")
ValueError: This is a test exception

"".join(traceback.format_exception_only(exc[0], exc[1])) - exception type and instance

ValueError: This is a test exception

[neiljp]

@rsashank This does work fine; my points are hopefully small and clear points to address 👍

The ExceptionView is fine 👍

[neiljp]

I temporarily changed this to C to work around the linter, and realized the UI didn't update, like it does with other commands. I think you're familiar with that approach from handling the about menu?

[neiljp]

Feature-wise this text is fine as it is 👍

That said, before merging I may explore adjusting these two parts since:

• they are two ways to obtain the same information
• we don't indicate that it is useful for the reporting mention in the section above it
• while we have precedent for the "Press 'c'" form, the rest of the app uses the [<key(s)>] style format

Overall, at least the first two points would make it clearer why we're mentioning where to find the exception details :)

[rsashank]

Got it, I'm a bit unsure about what you mean in the first two points, can you clarify on that?

Updated it to have the [c] format though.

[neiljp]

I'm still wary of including this change - but in any case if we do drop or include it, having it in this separate last commit will be useful either way 👍

I can see that the footer being taken-over (in addition to the UI via a popup) when there is a significant issue is reasonable, and emphasizes the seriousness of the matter, but I'm a little concerned that

• this couples the behavior in a way that users won't expect
• if the copying fails (no copy command available) we show an error popup, and then we don't clear the footer
• the message currently indicates that c is the only course of action
• if we add eg. Esc to the footer message too, then we're replicating some kind of contextual help ;)

[rsashank]

What do you think of this?

                if full_traceback != "":
                    text = [
                        "An exception occurred: ",
                        "Press [c] to copy traceback to clipboard.",
                    ]
                else:
                    text = ["Press [Esc] to exit popup."]

                self.view.set_footer_text(text, "task:error")

[neiljp]

Are you suggesting this addresses specific points?

In any case, it's unclear to me why you'd be checking full_traceback against the empty string?

[neiljp]

Reviewing the docs, I wonder if format_tb would be more useful?

The current *exc version is better for debugging than just the exception name, since we get a file (and line-number?), but it doesn't show as much of a full traceback.

I've not tested this, and it might need a followup to pass more data from the place where the exception arises, in order to work.

[rsashank]

I'm not entirely sure how to trigger an actual exception, but based on my own test exception. I believe format_exception would be the better option since it provides more information (type of the exception, exception message, full traceback)

format_exception

Traceback (most recent call last):
  File "/home/ubuntu/zulip-terminal/zulipterminal/ui_tools/boxes.py", line 827, in keypress
    raise ValueError("This is a test exception")
ValueError: This is a test exception

format_tb

  File "/home/ubuntu/zulip-terminal/zulipterminal/ui_tools/boxes.py", line 827, in keypress
    raise ValueError("This is a test exception")

[neiljp]

I took a quick look at the docs again, but let's leave it for now, and we can follow-up later if it doesn't provide enough information.

[rsashank]

@neiljp Thanks for the feedback, updated this PR 👍

[neiljp]

@rsashank Thanks for the updates 👍

I just pushed back here with the following changes:

• capitalized the Exception Popup (similar to Niloth's changes) in the help
• removed an additional default string parameter
• used a dynamic key for COPY_TRACEBACK (as per Copy traceback from the Exception popup #1513 (comment))

I also have dropped the last commit, since it doesn't appear to work properly (eg. a recent update of key name(s) causes a crash) and we discussed previously that it might be something not to include, at least in this iteration.

I tested manually by raising an exception in the event loop try block, where the code is normally called.

I'll push the minor textual changes I had in mind separately, after merging this now 🎉