Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Cross-Plattform Crash Reporting #4455
Cross-Plattform Crash Reporting #4455
Changes from all commits
d5afd95
c248adb
5d118dc
dd2f9d8
d8fbfb9
39b6523
7d00bf6
2493b1f
0e7fe2d
dfae022
3ca44c3
d6c0a95
31a86e6
a1ab039
138ff48
60d5e10
d3d5126
b279b62
5d480e2
60e95ce
46dd859
2c87cb0
25b3023
1bfe91a
891a0f9
9b52e63
c4ff6a2
21664db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why no iOS and Linux?
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.
Not tested so far, as i don't have a build machine setuped yet 😅.
For iOS I want to wait for it to be moved to cmake before i start that.
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.
Heh, totally get that. Let's file tickets for the iOS and Linux work though, so we can keep track of it.
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.
nit: Can we use an elseif here? Also for the Android conditional.
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.
nit: I'd remove this message. It's redudant with the previous one and it looks like an error.
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.
nit: newline.
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.
Question: What are we removing here? What is the crash client? How is this related to the Sentry changes?
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.
The crash client is a mini qt application we're shipping in the client. It shows the "ohno it's crashed, wanna upload?" prompt. It's currently only running under windows.
This line will make the crash client register itself for "structured exception handling", - which we don't want as sentry will do that.
On that note, I currently don't want to rip that whole thing out.... yet. We might still want to create the crash-ui process, in the sentry onCrash handler.
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.
Alright, yeah my question was a precursor to me asking if you wanted to also just remove the whole thing. Thanks for clarifying!