Fixed crash reporting U.X. #1507

Penguin-Guru · 2021-12-02T07:37:38Z

#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.

…let app. Slight change to app setting text.

Penguin-Guru · 2021-12-02T08:09:06Z

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).

vircadia-build-notifier · 2021-12-02T09:41:16Z

The following links are available:
build (macOS-10.15, client)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR1507/Vircadia-Interface-PR1507-b8a2c8c-_8cd0ca5d.dmg

build (macOS-10.15, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR1507/Vircadia-PR1507-b8a2c8c-_8cd0ca5d.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR1507/Vircadia-PR1507-b8a2c8c-_8cd0ca5d.exe

build (self-hosted_debian-11_aarch64, full)

JulianGro · 2021-12-02T14:31:07Z

interface/src/ui/PreferencesDialog.cpp

-                                "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.


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.

Nevermind just saw your comment. Maybe the code comment should state that this only affects the UI and not the saved setting.

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.

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.

Penguin-Guru · 2021-12-03T09:00:58Z

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.

JulianGro · 2021-12-03T17:13:19Z

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.

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.

Penguin-Guru · 2021-12-03T17:17:17Z

Ok. I'll give it a shot.

Penguin-Guru · 2021-12-03T21:56:10Z

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:

ToolTip opacity not being set when I attempted to hover.
Positioning of tooltip text was not what I wanted.

There may be more efficient ways of adding the warning to the Q.M.L. I had not yet fully explored that.

vircadia-build-notifier · 2021-12-04T02:43:52Z

The following links are available:
build (macOS-10.15, client)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR1507/Vircadia-Interface-PR1507-718f6d9-_e82e6ae6.dmg

build (macOS-10.15, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR1507/Vircadia-PR1507-718f6d9-_e82e6ae6.dmg

build (ubuntu-18.04, full)

build (windows-latest, full)

https://athena-public.s3.eu-west-3.amazonaws.com/builds/PR/vircadia_vircadia/PR1507/Vircadia-PR1507-718f6d9-_e82e6ae6.exe

build (self-hosted_debian-11_aarch64, full)

daleglass · 2021-12-18T19:01:08Z

interface/resources/qml/controlsUit/ToolTip.qml

@@ -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.


Why doesn't it work?

Not sure if I am looking at this right, but is 1 : 0 a division by zero?

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.

daleglass · 2021-12-18T19:01:49Z

libraries/shared/src/Preferences.h

@@ -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; }
+


Why is this being added?

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.

stale · 2022-06-18T01:47:19Z

Hello! Is this still an issue?

Fixed crash reporting U.X. and rephrased toolbar setting to match tab…

8cd0ca5

…let app. Slight change to app setting text.

Penguin-Guru changed the title ~~Fixed crash reporting U.X. (#1484)~~ Fixed crash reporting U.X. Dec 2, 2021

digisomni added allow-build-upload Allows the upload of a build for non white-listed users enhancement New feature or request needs CR (code review) labels Dec 2, 2021

digisomni added this to In progress in 2022.1.1 Selene Release via automation Dec 2, 2021

digisomni modified the milestones: 2022.1.0 Selene Release, 2022.1.1 Selene Release Dec 2, 2021

JulianGro reviewed Dec 2, 2021

View reviewed changes

Adjusted comment for clarity.

faf77ce

Removed disabling, added warning icon. Has bugs.

e82e6ae

daleglass added the needs adoption Please adopt me! I've been abandoned. label Dec 11, 2021

daleglass suggested changes Dec 18, 2021

View reviewed changes

2022.1.1 Selene Release automation moved this from In progress to Review in progress Dec 18, 2021

Penguin-Guru mentioned this pull request May 22, 2022

Fix Crash Reporting U.X. overte-org/overte#67

Draft

stale bot added the stale Issue / PR has not had activity label Jun 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed crash reporting U.X. #1507

Fixed crash reporting U.X. #1507

Penguin-Guru commented Dec 2, 2021 •

edited

Penguin-Guru commented Dec 2, 2021 •

edited

vircadia-build-notifier commented Dec 2, 2021

JulianGro Dec 2, 2021

JulianGro Dec 2, 2021 •

edited

Penguin-Guru Dec 2, 2021 •

edited

Penguin-Guru Dec 2, 2021 •

edited

Penguin-Guru commented Dec 3, 2021

JulianGro commented Dec 3, 2021 •

edited

Penguin-Guru commented Dec 3, 2021

Penguin-Guru commented Dec 3, 2021 •

edited

vircadia-build-notifier commented Dec 4, 2021

daleglass Dec 18, 2021

JulianGro Dec 19, 2021 •

edited

Penguin-Guru Dec 19, 2021

daleglass Dec 18, 2021

Penguin-Guru Dec 19, 2021

stale bot commented Jun 18, 2022

Fixed crash reporting U.X. #1507

Are you sure you want to change the base?

Fixed crash reporting U.X. #1507

Conversation

Penguin-Guru commented Dec 2, 2021 • edited

Penguin-Guru commented Dec 2, 2021 • edited

vircadia-build-notifier commented Dec 2, 2021

JulianGro Dec 2, 2021

Choose a reason for hiding this comment

JulianGro Dec 2, 2021 • edited

Choose a reason for hiding this comment

Penguin-Guru Dec 2, 2021 • edited

Choose a reason for hiding this comment

Penguin-Guru Dec 2, 2021 • edited

Choose a reason for hiding this comment

Penguin-Guru commented Dec 3, 2021

JulianGro commented Dec 3, 2021 • edited

Penguin-Guru commented Dec 3, 2021

Penguin-Guru commented Dec 3, 2021 • edited

vircadia-build-notifier commented Dec 4, 2021

daleglass Dec 18, 2021

Choose a reason for hiding this comment

JulianGro Dec 19, 2021 • edited

Choose a reason for hiding this comment

Penguin-Guru Dec 19, 2021

Choose a reason for hiding this comment

daleglass Dec 18, 2021

Choose a reason for hiding this comment

Penguin-Guru Dec 19, 2021

Choose a reason for hiding this comment

stale bot commented Jun 18, 2022

Penguin-Guru commented Dec 2, 2021 •

edited

Penguin-Guru commented Dec 2, 2021 •

edited

JulianGro Dec 2, 2021 •

edited

Penguin-Guru Dec 2, 2021 •

edited

Penguin-Guru Dec 2, 2021 •

edited

JulianGro commented Dec 3, 2021 •

edited

Penguin-Guru commented Dec 3, 2021 •

edited

JulianGro Dec 19, 2021 •

edited