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

GTK: crash reports notice #4387

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

maciekbartczak
Copy link
Contributor

This pr implements the GUI notice informing about existing crash reports in the GTK application, and partially resolves #2183.

As proposed in #2183 (comment) I implemented a banner similiar to the debug mode notice.
The message embeds clickable links leading to the reports directory and reporting instructions.

gtk-adwaita=true:
image
gtk-adwaita=false:
image

@maciekbartczak maciekbartczak force-pushed the gtk-crash-reports-notice branch from 682c088 to fc24bcb Compare January 2, 2025 17:13
@mitchellh
Copy link
Contributor

This looks pretty good to me. I'd like @jcollie or @tristan957 to review this.

We're now disabling Sentry by default on Linux because its not providing useful information yet, but having this in for when its enabled would be good.

@tristan957
Copy link
Collaborator

I don't like that we seem to be controlling the line break. I'd rather let GTK/Pango handle that, so the text can just flow as needed.

@jcollie
Copy link
Collaborator

jcollie commented Jan 3, 2025

I don't like that we seem to be controlling the line break. I'd rather let GTK/Pango handle that, so the text can just flow as needed.

I tried that out and it doesn't look very good, plus it takes up a lot more vertical space because it's not packed very well. Unless we get fancier with the layout I think it's a good compromise.

image

@mitchellh
Copy link
Contributor

I don't like that we seem to be controlling the line break. I'd rather let GTK/Pango handle that, so the text can just flow as needed.

Fwiw on the macOS we manually control line breaking all over to be more aesthetically pleasing.

Copy link
Collaborator

@jcollie jcollie left a comment

Choose a reason for hiding this comment

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

The code also needs to have zig fmt run against it. I've made a few comments, but otherwise is looking good. prettier also needs to be run to format the .css files.

src/apprt/gtk/Window.zig Outdated Show resolved Hide resolved
self.app.core_app.alloc,
\\ There are existing crash reports located at <a href="file://{s}">{s}</a>.
\\ If possible, <a href="https://github.com/ghostty-org/ghostty?tab=readme-ov-file#crash-reports">report them to the developers</a> to help improve the application.
,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's wonderful that clicking the links takes you someplace useful. I was thinking that we'd immediately be inundated with requests to get some kind of GUI to manage crash reports (and we may still need that eventually) but clicking the link brings up Gnome's file manager with the crash reports directory so it's pretty easy for users to manage their crash reports.

src/apprt/gtk/Window.zig Outdated Show resolved Hide resolved
src/apprt/gtk/Window.zig Outdated Show resolved Hide resolved
@maciekbartczak maciekbartczak requested a review from jcollie January 3, 2025 08:56
@maciekbartczak maciekbartczak force-pushed the gtk-crash-reports-notice branch from b0ba5ab to d58458a Compare January 7, 2025 18:29
@maciekbartczak
Copy link
Contributor Author

Hey @jcollie, just following up on this PR - I've addressed the requested changes and resolved the conflicts, I would appreciate a re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GUI notice there are crash reports
4 participants