Fixed crash reporting U.X. #1507
Conversation
…let app. Slight change to app setting text.
Penguin-Guru
changed the title
digisomni
added
allow-build-upload
|
Note that this is designed to de-sync the app setting display from the toolbar setting (and actual setting saved), to reflect the actual behaviour without changing the saved setting. This means it is possible for a user to compile without crash reporting, believe that their crash reporting setting is disabled based on the app U.I., port their settings over to a build that does support crash reporting, and have their crash reports surprisingly submitted. This is not ideal but it seems like a fringe case and is largely eliminated if the default value is unchecked. I designed it like this to prevent the case where the setting is disabled and checked, which would suggest that crash reporting is functional and can not be turned off. I couldn't think of another good way to communicate that the functionality is disabled. Maybe a tool-tip saying "if checkbox is disabled, functionality is too"? That seems a bit rough though. Including it in the text description also seems rough. This would be less of an issue with a tool-tip but disabled checkboxes look almost identical to enabled checkboxes when using the dark theme (#1508). I'm not confident enough in my artistic skills to edit the theme(s). |
|
The following links are available: build (macOS-10.15, full) build (ubuntu-18.04, full)
build (windows-latest, full) build (self-hosted_debian-11_aarch64, full)
|
| "this information you are helping to improve the product. ", getter, setter)); | ||
| auto getter = []()->bool { return | ||
| Menu::getInstance()->isOptionChecked(MenuOption::GenerateAndSubmitCrashReports) | ||
| && UserActivityLogger::getInstance().isCrashMonitorEnabled() // Reset to unchecked if feature is disabled so users aren't locked in. |
There was a problem hiding this comment.
Resetting to unchecked seems like a bad idea.
If someone enables crash reporting they will most likely have it enabled wherever possible. If they then run a build that doesn't come with crash reporting, it will disable their setting without them knowing. We already have fairly little people that turn the option on, so I would hate so have it turn off for them automatically somehow.
There was a problem hiding this comment.
Nevermind just saw your comment. Maybe the code comment should state that this only affects the UI and not the saved setting.
There was a problem hiding this comment.
This only resets the checkbox, not the setting. It may reset the setting if the user saves with the checkbox unchecked or it may not (I can't test this). The taskbar checkbox indicates the true value of the setting (which I have mixed feelings about). Do you have a better suggestion? I'm going to update my previous comment on this PR to correct the problem statement.
There was a problem hiding this comment.
Oh, your second comment didn't appear until I opened the page in a new tab... Yes, I will adjust that comment for clarity but my hope was that users could save the unchecked checkbox to disable the setting if they wanted to or cancel to not disable it. I know that's a bit problematic but it's the best solution I could think of. Another option would be to never disable the setting in the toolbar but I don't think it's good to rely on the toolbar.
|
I have the tool-tip mostly set up but I'm starting to think it would be better to not disable anything and just add some kind of warning icon with a tool-tip near the affected checkbox when the functionality is disabled. A dark red exclamation mark in a ring border comes to mind but I don't really want to make that and figure out how to add it to the Q.M.L. Maybe I will tomorrow. |
There has to be a warning symbol floating around. A white triangle with an exclamation mark in it. It's used in the asset browser when there is an issue. And yeah I definitely like that a lot more than disabling things. |
|
Ok. I'll give it a shot. |
|
Unfortunately, I lost my motivation to work on this. I'm going to push what I was working on so someone can fix the bugs or close this P.R. Bugs were:
There may be more efficient ways of adding the warning to the Q.M.L. I had not yet fully explored that. |
|
The following links are available: build (macOS-10.15, full) build (ubuntu-18.04, full)
build (windows-latest, full) build (self-hosted_debian-11_aarch64, full)
|
| @@ -20,7 +22,8 @@ Item { | |||
|
|
|||
| width: toolTipText.width + 4 | |||
| height: toolTipText.height + 4 | |||
| opacity: (toolTip != "" && showToolTip) ? 1 : 0 | |||
| opacity: (toolTip != "" && showToolTip) ? 1 : 0 // Doesn't work. | |||
| //opacity: 1 // Works. | |||
There was a problem hiding this comment.
Not sure if I am looking at this right, but is 1 : 0 a division by zero?
There was a problem hiding this comment.
Julian: No, '?' is a ternary operator. https://en.wikipedia.org/wiki/%3F:#C++
Dale: I have no idea. That's why I couldn't get it to work. See my previous comment on this P.R. I tried several things to isolate the issue but had to give up before figuring it out. Sorry.
| @@ -391,10 +393,21 @@ class CheckPreference : public BoolPreference { | |||
| : BoolPreference(category, name, getter, setter) { } | |||
| Type getType() override { return Checkbox; } | |||
|
|
|||
| //bool getEnabled() { return _isEnabled; } | |||
| //void setEnabled(const bool enabled) { _isEnabled = enabled; } | |||
|
|
|||
There was a problem hiding this comment.
Why is this being added?
There was a problem hiding this comment.
Why is this being added?
I was using that for a previous approach and decided to leave it as a comment rather than taking it out, since it might still be useful at some point. You can remove it if you want.
|
Hello! Is this still an issue? |
#1484
I obviously can't test this so hopefully it works.
Also rephrased toolbar setting to match tablet app and made a slight change to app setting text.