Skip to content

Fixed crash reporting U.X. #1507

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.

Sign up for GitHub

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

Open
Penguin-Guru wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fixed crash reporting U.X. #1507

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cd0ca5
Fixed crash reporting U.X. and rephrased toolbar setting to match tab…
Dec 2, 2021
faf77ce
Adjusted comment for clarity.
Dec 2, 2021
e82e6ae
Removed disabling, added warning icon. Has bugs.
Dec 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions interface/resources/qml/dialogs/preferences/CheckBoxPreference.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Preference {
height: spacer.height + Math.max(hifi.dimensions.controlLineHeight, checkBox.implicitHeight)
property bool value: false
Component.onCompleted: {
checkBox.enabled = preference.enabled;
checkBox.checked = preference.value;
value = checkBox.checked;
preference.value = Qt.binding(function(){ return checkBox.checked; });
Expand Down
18 changes: 12 additions & 6 deletions interface/src/Menu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,12 +633,18 @@ Menu::Menu() {
true,
&UserActivityLogger::getInstance(),
SLOT(disable(bool)));
addCheckableActionToQMenuAndActionHash(networkMenu,
MenuOption::DisableCrashLogger,
0,
true,
&UserActivityLogger::getInstance(),
SLOT(crashMonitorDisable(bool)));
{
auto ual = &UserActivityLogger::getInstance();
addCheckableActionToQMenuAndActionHash(networkMenu,
MenuOption::GenerateAndSubmitCrashReports,
0,
true,
ual,
SLOT(crashMonitorDisable(bool)),
UNSPECIFIED_POSITION,
QString(),
ual->isCrashMonitorEnabled());
}
addActionToQMenuAndActionHash(networkMenu, MenuOption::ShowDSConnectTable, 0,
qApp, SLOT(loadDomainConnectionDialog()));

Expand Down
2 changes: 1 addition & 1 deletion interface/src/Menu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ namespace MenuOption {
const QString DeleteAvatarEntitiesBookmark = "Delete Avatar Entities Bookmark";
const QString DeleteBookmark = "Delete Bookmark...";
const QString DisableActivityLogger = "Disable Activity Logger";
const QString DisableCrashLogger = "Disable Crash Reporter";
const QString DisableEyelidAdjustment = "Disable Eyelid Adjustment";
const QString DisableLightEntities = "Disable Light Entities";
const QString DisplayCrashOptions = "Display Crash Options";
Expand Down Expand Up @@ -123,6 +122,7 @@ namespace MenuOption {
const QString FixGaze = "Fix Gaze (no saccade)";
const QString Forward = "Forward";
const QString FrameTimer = "Show Timer";
const QString GenerateAndSubmitCrashReports = "Generate and Submit Crash Reports";
const QString Help = "Help...";
const QString HomeLocation = "Home ";
const QString IncreaseAvatarSize = "Increase Avatar Size";
Expand Down
12 changes: 7 additions & 5 deletions interface/src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,11 +220,13 @@ int main(int argc, const char* argv[]) {
}
qDebug() << "UserActivityLogger is enabled:" << ual.isEnabled();

bool isCrashHandlerEnabled = ual.isCrashMonitorEnabled() || parser.isSet(forceCrashReportingOption);
qDebug() << "Crash handler logger is enabled:" << isCrashHandlerEnabled;
if (isCrashHandlerEnabled) {
auto crashHandlerStarted = startCrashHandler(argv[0]);
qDebug() << "Crash handler started:" << crashHandlerStarted;
{
bool isCrashHandlerEnabled = ual.isCrashMonitorEnabled() || parser.isSet(forceCrashReportingOption);
qDebug() << "Crash handler logger is enabled:" << isCrashHandlerEnabled;
if (isCrashHandlerEnabled) {
auto crashHandlerStarted = startCrashHandler(argv[0]);
qDebug() << "Crash handler started:" << crashHandlerStarted;
}
}

const QString& applicationName = getInterfaceSharedMemoryName();
Expand Down
15 changes: 10 additions & 5 deletions interface/src/ui/PreferencesDialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,11 +257,16 @@ void setupPreferences() {
}

{
auto getter = []()->bool { return !Menu::getInstance()->isOptionChecked(MenuOption::DisableCrashLogger); };
auto setter = [](bool value) { Menu::getInstance()->setIsOptionChecked(MenuOption::DisableCrashLogger, !value); };
preferences->addPreference(new CheckPreference("Privacy", "Send crashes - Vircadia uses information provided by your "
"client to improve the product through crash reports. By allowing Vircadia to collect "
"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.

@JulianGro JulianGro Dec 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@JulianGro JulianGro Dec 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@Penguin-Guru Penguin-Guru Dec 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@Penguin-Guru Penguin-Guru Dec 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

;};
auto setter = [](bool value) { Menu::getInstance()->setIsOptionChecked(MenuOption::GenerateAndSubmitCrashReports, !value); };
auto preference = new CheckPreference("Privacy", "Send crashes - Vircadia can use information provided by your "
"client to improve the product through crash reports. By allowing developers to collect "
"this information you are helping to improve the product. ", getter, setter);
preference->setEnabled(UserActivityLogger::getInstance().isCrashMonitorEnabled());
preferences->addPreference(preference);
}

static const QString AVATAR_TUNING { "Avatar Tuning" };
Expand Down
2 changes: 1 addition & 1 deletion libraries/networking/src/UserActivityLogger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public slots:
bool isDisabledSettingSet() const { return _disabled.isSet(); }

bool isCrashMonitorEnabled() { return !_crashMonitorDisabled.get(); }
bool isCrashMonitorDisabledSettingSet() const { return _crashMonitorDisabled.isSet(); }
bool isCrashMonitorDisabledSettingSet() const { return _crashMonitorDisabled.isSet(); } // Is this actually used?

void disable(bool disable);
void crashMonitorDisable(bool disable);
Expand Down
5 changes: 5 additions & 0 deletions libraries/shared/src/Preferences.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ class AvatarPreference : public BrowsePreference {

class CheckPreference : public BoolPreference {
Q_OBJECT
Q_PROPERTY(bool enabled READ getEnabled CONSTANT)
Q_PROPERTY(bool indented READ getIndented CONSTANT)
public:
using Getter = std::function<bool()>;
Expand All @@ -391,9 +392,13 @@ 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; }

@daleglass daleglass Dec 18, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this being added?

@Penguin-Guru Penguin-Guru Dec 19, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

bool getIndented() { return _isIndented; }
void setIndented(const bool indented) { _isIndented = indented; }
protected:
bool _isEnabled { true };
bool _isIndented { false };
};

Expand Down
7 changes: 5 additions & 2 deletions libraries/ui/src/ui/Menu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,12 +208,14 @@ QAction* Menu::addCheckableActionToQMenuAndActionHash(MenuWrapper* destinationMe
const QObject* receiver,
const char* member,
int menuItemLocation,
const QString& grouping) {
const QString& grouping,
const bool enabled) {

QAction* action = addActionToQMenuAndActionHash(destinationMenu, actionName, shortcut, receiver, member,
QAction::NoRole, menuItemLocation);
action->setCheckable(true);
action->setChecked(checked);
action->setDisabled(!enabled);

if (isValidGrouping(grouping)) {
_groupingActions[grouping] << action;
Expand All @@ -229,7 +231,8 @@ QAction* Menu::addCheckableActionToQMenuAndActionHash(MenuWrapper* destinationMe
const QKeySequence& shortcut,
const bool checked,
int menuItemLocation,
const QString& grouping) {
const QString& grouping) {

auto action = addCheckableActionToQMenuAndActionHash(destinationMenu, actionName, shortcut, checked, nullptr, nullptr, menuItemLocation, grouping);
connect(action, &QAction::triggered, handler);
return action;
Expand Down
3 changes: 2 additions & 1 deletion libraries/ui/src/ui/Menu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ class Menu : public QMenuBar {
const QObject* receiver = NULL,
const char* member = NULL,
int menuItemLocation = UNSPECIFIED_POSITION,
const QString& grouping = QString());
const QString& grouping = QString(),
const bool enabled = true);

QAction* addCheckableActionToQMenuAndActionHash(MenuWrapper* destinationMenu,
const QString& actionName,
Expand Down