[GUI] Tune advanced preferences dialog #2585

RichardHaselgrove · 2018-07-02T15:07:03Z

Following user feedback on v7.10.2, minor adjustments to #2043

Make 'Save' the default button if (and only if) the user already has local preferences active
Show confirmation dialog when (and only when) switching from web to local preferences

The two commits in this PR are independent and either could be deployed separately, but I think they work better together.

Discussion and justification in CANCEL as default on options change

@CharlieFenton - specifically requesting your review to ensure it works on other platforms too - I've only tested Windows.

Tweak #2043. With users encountering these changes for the first time in 7.10, there are complaints about the new behaviour. This restores the Save button to be default if the user is ALREADY using local preferences: keeps cancel as default if the user is currently using web preferences.

Add confirmation when (and only when) switching from web to local preferences.

RichardHaselgrove · 2018-07-02T15:14:27Z

While working on this PR, I tested the 'Help' button - it's a mess. The code here is fine, but the target is a Wiki page...
...which is a manual redirect...
...to a page which doesn't exist...
...but wiki automation suggests a third page...
...which has had all the 'help' text deleted...
...leaving just screenshots...
...which are out of date.

If the events of 14 February 2017 weren't recorded in the final history log, I'd think we'd been hacked.

Ageless93 · 2018-07-02T15:18:21Z

It's not settings. It's preferences. It's local preferences, not local settings. You speak of web-based preferences, so why use settings for the local ones?

The present blurb at the top of the window says:
"Using local preferences. Click "Use web prefs" to use web-based preferences from {project-url}"
or
"Using web-based preferences from {project-url}. Set values and click Save to use local preferences instead."

So keep the same text all over.

Correcting terminology in confirmation dialog

CharlieFenton · 2018-07-03T09:26:39Z

Show confirmation dialog when (and only when) switching from web to local preferences

While I (just now) read the forum thread discussion and understand the reason for a confirmation dialog when switching to local prefs, I think it is far more important to have a confirmation dialog when switching from local to web prefs.

Depending on the complexity of one's local preferences, it can take quite a bit of time and effort to set them up. But you can wipe out all that work with one button click on the Use web prefs button. After doing that, the only way to get back what you just lost is to painstakingly go through the 4 pages of dialogs again (assuming you had a complex set of local preferences.)

CharlieFenton · 2018-07-03T09:50:56Z

I have confirmed that the code changes under this PR work as intended on my Macintosh.

I think it is far more important to have a confirmation dialog when switching from local to web prefs.

I had forgotten that there already was a confirmation dialog when switching from local to web prefs, and I now see that this PR does not change that. But I think that the local to web prefs confirmation could be somewhat more emphatic that your local prefs will be lost and you won't be able to "undo" the decision.

TheAspens · 2018-07-03T13:11:48Z

It looks like scruitinizer errored out. I"m closing and reopeninging this pull request in order to retrigger it.

RichardHaselgrove · 2018-07-04T10:40:32Z

I had forgotten that there already was a confirmation dialog...

Yes, I saw that while working on it - that's partly what triggered my comment about BOINC's terse style in the discussion. I didn't feel the need to change it, because the button you click is much more specific - less likelihood of being unaware of the consequences. But we could use the opportunity to make the two styles more similar:

"Changing to use web preferences will delete the local preferences from all four pages of this dialog. You won't be able to 'undo' this operation later. Do you want to proceed?"

It would make life easier for the translation team if we make both changes together, if at all.

…_AdvPrefDlg

JuhaSointusalo

Please configure your editor to use four spaces for indentation and correct the indentation in the code you added.

JuhaSointusalo · 2018-07-07T16:34:31Z

clientgui/DlgAdvPreferencesBase.cpp

@@ -37,6 +37,8 @@
 #define STATICBOXVERTICALSPACER 10
 #define DAYOFWEEKBORDERSIZE 10

+bool usingLocalPrefs;


Make this a member variable instead (move to header inside class and rename to m_bUsingLocalPrefs).

I'm afraid I've completely failed to work out how to add a boolean value to the class definition - I only need to export one lousy bit! It should be wxBool or something, but searching the online manual only finds wxBook or books about wool.

JuhaSointusalo · 2018-07-07T16:36:09Z

clientgui/DlgAdvPreferencesBase.cpp

@@ -1080,6 +1085,11 @@ bool CDlgAdvPreferencesBase::doesLocalPrefsFileExist() {
    return local_prefs_found;
 }

+// to make result available externally
+bool CDlgAdvPreferencesBase::isUsingPrivatePrefs() {


I don't think I've heard them called private prefs before. Maybe isUsingLocalPrefs instead?

You're right. Changed.

JuhaSointusalo · 2018-07-07T16:38:16Z

clientgui/DlgAdvPreferences.cpp

+bool CDlgAdvPreferences::ConfirmSetLocal() {
+    wxString strMessage     = wxEmptyString;
+	strMessage.Printf(
+            _("Changing to use the local BOINC preferences defined on this page. BOINC will ignore your web-based preferences, even if you subsequently make changes there. Do you want to proceed?")


@CharlieFenton Does this need branding?
@RichardHaselgrove Or maybe you can find a good wording that doesn't mention BOINC?

"Changing to use" is awkward.
Can omit "BOINC".
Can omit "defined on this page".
Could say "This will override web-based preferences".

I've removed the need for branding, and used override instead of ignore. I'd like to keep 'defined on this page', because this whole PR is addressed at users encountering the difference between web and local preferences for the first and only time.

I haven't found a better alternative for 'Changing to use', but it's up for better suggestions.

RichardHaselgrove · 2018-07-07T20:33:46Z

It's always better if you don't try to write the words in the same session as wrestling with the code. I'll look at both of these in the morning.

CharlieFenton · 2018-07-07T21:13:45Z

Does this need branding?

Yes; anywhere "BOINC" or "BOINC Manager" appears in the UI, it needs to allow for branding.

JuhaSointusalo · 2018-07-08T20:21:54Z

@RichardHaselgrove

I forgot earlier. Could you make the same changes to simple GUI?

Changes requested by reviewers: Corrected space for tab Changed function name Removed branding from dialog

RichardHaselgrove · 2018-07-12T11:06:44Z

I've sorted out 'spaces for tabs', and found out where Notepad++ keeps the default - so many standards to choose from! The global tidyup has caught several more instances.

I'll do the simple GUI version as a separate PR later, when we're all happy with this one.

…_AdvPrefDlg

JuhaSointusalo · 2018-07-12T14:28:31Z

I'm afraid I've completely failed to work out how to add a boolean value to the class definition

Like so

diff --git a/clientgui/DlgAdvPreferencesBase.cpp b/clientgui/DlgAdvPreferencesBase.cpp
index 704d90d4b7..5ffa0ce0b1 100644
--- a/clientgui/DlgAdvPreferencesBase.cpp
+++ b/clientgui/DlgAdvPreferencesBase.cpp
@@ -37,8 +37,6 @@
 #define STATICBOXVERTICALSPACER 10
 #define DAYOFWEEKBORDERSIZE 10
 
-bool usingLocalPrefs;
-
 ///////////////////////////////////////////////////////////////////////////
 
 // NOTE: On MS Windows with wxWidgets 3.0, controls inside a wxStaticBox 
@@ -63,7 +61,7 @@ CDlgAdvPreferencesBase::CDlgAdvPreferencesBase( wxWindow* parent, int id, wxStri
     wxBoxSizer* dialogSizer = new wxBoxSizer( wxVERTICAL );
 
 
-    usingLocalPrefs = doesLocalPrefsFileExist();
+    m_bUsingLocalPrefs = doesLocalPrefsFileExist();
     if (web_prefs_url->IsEmpty()) {
         m_bmpWarning = NULL;
     } else {
@@ -78,7 +76,7 @@ CDlgAdvPreferencesBase::CDlgAdvPreferencesBase( wxWindow* parent, int id, wxStri
 
         wxBoxSizer* legendSizer = new wxBoxSizer( wxVERTICAL );
 
-        if (usingLocalPrefs) {
+        if (m_bUsingLocalPrefs) {
             legendSizer->Add(
                 new wxStaticText( topControlsStaticBox, ID_DEFAULT,
                             _("Using local preferences.\n"
@@ -103,7 +101,7 @@ CDlgAdvPreferencesBase::CDlgAdvPreferencesBase( wxWindow* parent, int id, wxStri
             0, wxLEFT, 5
         );
         
-        if (!usingLocalPrefs) {
+        if (!m_bUsingLocalPrefs) {
             legendSizer->Add(
                 new wxStaticText( topControlsStaticBox, ID_DEFAULT,
                      _("Set values and click Save to use local preferences instead."),
@@ -116,7 +114,7 @@ CDlgAdvPreferencesBase::CDlgAdvPreferencesBase( wxWindow* parent, int id, wxStri
 
         m_btnClear = new wxButton( topControlsStaticBox, ID_BTN_CLEAR, _("Use web prefs"), wxDefaultPosition, wxDefaultSize, 0 );
         m_btnClear->SetToolTip( _("Restore web-based preferences and close the dialog.") );
-        if (!usingLocalPrefs) {
+        if (!m_bUsingLocalPrefs) {
             m_btnClear->Hide();
         }
         
@@ -163,14 +161,14 @@ CDlgAdvPreferencesBase::CDlgAdvPreferencesBase( wxWindow* parent, int id, wxStri
 
     m_btnOK = new wxButton( m_panelButtons, wxID_OK, _("Save"), wxDefaultPosition, wxDefaultSize, 0 );
     m_btnOK->SetToolTip( _("Save all values and close the dialog.") );
-    if (usingLocalPrefs) {
+    if (m_bUsingLocalPrefs) {
         m_btnOK->SetDefault();
     }
     buttonSizer->Add( m_btnOK, 0, wxALL, 5 );
 
     m_btnCancel = new wxButton( m_panelButtons, wxID_CANCEL, _("Cancel"), wxDefaultPosition, wxDefaultSize, 0 );
     m_btnCancel->SetToolTip( _("Close the dialog without saving.") );
-    if (!usingLocalPrefs) {
+    if (!m_bUsingLocalPrefs) {
         m_btnCancel->SetDefault();
     }
     buttonSizer->Add( m_btnCancel, 0, wxALL, 5 );
@@ -1087,7 +1085,7 @@ bool CDlgAdvPreferencesBase::doesLocalPrefsFileExist() {
 
 // to make result available externally
 bool CDlgAdvPreferencesBase::isUsingLocalPrefs() {
-    return usingLocalPrefs;
+    return m_bUsingLocalPrefs;
 }
 
 void CDlgAdvPreferencesBase::makeStaticBoxLabelItalic(wxStaticBox* staticBox) {
diff --git a/clientgui/DlgAdvPreferencesBase.h b/clientgui/DlgAdvPreferencesBase.h
index 42a3d1cd42..d7fa4a7607 100644
--- a/clientgui/DlgAdvPreferencesBase.h
+++ b/clientgui/DlgAdvPreferencesBase.h
@@ -243,6 +243,7 @@ protected:
     wxButton* m_btnHelp;
     
     wxString *web_prefs_url;
+    bool m_bUsingLocalPrefs;
 
 public:
     CDlgAdvPreferencesBase( wxWindow* parent, int id = -1, wxString title = wxT(""), wxPoint pos = wxDefaultPosition, wxSize size = wxDefaultSize, int style = wxDEFAULT_DIALOG_STYLE );

JuhaSointusalo · 2018-07-12T14:29:09Z

I'll do the simple GUI version as a separate PR later, when we're all happy with this one.

Ok.

JuhaSointusalo · 2018-07-12T14:31:41Z

About naming variables. BOINC code base isn't too consistent in that but I think this m_bUsingLocalPrefs style is used more often in Manager code than other styles.

Use class to pass variable, function not needed Rename previous local variable to class format

Fix typo causing appveyor error

RichardHaselgrove · 2018-07-12T18:58:33Z

Thanks @JuhaSointusalo - that's done it, and we can lose the new function entirely. I don't know how I missed that permutation, but everything I tried threw up a compile error - mind you, it was unthinkably hot in my workroom earlier in the week.

The logic in this PR is now ready for review - only the confirmation wording is still open for improvement. Appveyor caught a typo in copying from test to commit branch locally - fixed and passed by appveyor.

JuhaSointusalo · 2018-07-14T15:49:38Z

Code looks fine. I'm not merging yet because I don't know if we have reached an agreement on the wording.

clientgui/DlgAdvPreferences.h

clientgui/DlgAdvPreferencesBase.cpp

JuhaSointusalo · 2018-09-08T15:18:20Z

The current wording is

Changing to use the local preferences defined on this page. This will override your web-based preferences, even if you subsequently make changes there. Do you want to proceed?

Nobody has said they don't like it so maybe we can assume everyone is happy with it and proceed with getting this PR merged.

@RichardHaselgrove

@AenBleidd made some requests. Could you address them?

AenBleidd

I'm OK with this pull request

RichardHaselgrove · 2018-09-08T17:15:25Z

I think @AenBleidd and I went through all the queries and agreed at least a reason for leaving them as they stand. The wording was discussed in the message board thread and, again, although there were minor differences of opinion, everyone accepted that the current wording was close enough. I'd like to think we could merge and move on to the simple GUI case?

RichardHaselgrove · 2018-09-09T20:55:32Z

Thanks. I'll work on the simple GUI tomorrow (Monday 10 Sept) and hope to have a PR ready the same day - it would be good to have both versions in sync before 7.14 is branched.

Changes match those applied to the advanced dialog in #2585: Make 'Save' the default button if (and only if) the user already has local preferences active Show confirmation dialog when (and only when) switching from web to local preferences

RichardHaselgrove added 2 commits July 1, 2018 14:39

[GUI] Tune advanced preferences dialog (2)

c4201d1

Add confirmation when (and only when) switching from web to local preferences.

RichardHaselgrove requested review from AenBleidd and CharlieFenton July 2, 2018 15:07

[GUI] Tune advanced preferences dialog (3)

aa03959

Correcting terminology in confirmation dialog

TheAspens closed this Jul 3, 2018

TheAspens reopened this Jul 3, 2018

Merge branch 'master' of https://github.com/BOINC/boinc into RDH_tune…

e84b020

…_AdvPrefDlg

JuhaSointusalo reviewed Jul 7, 2018

View reviewed changes

[GUI] Tune advanced preferences dialog (4)

ad97779

Changes requested by reviewers: Corrected space for tab Changed function name Removed branding from dialog

Merge branch 'master' of https://github.com/BOINC/boinc into RDH_tune…

67fb051

…_AdvPrefDlg

RichardHaselgrove added 2 commits July 12, 2018 19:22

[GUI] Tune advanced preferences dialog (5)

1d94d2d

Use class to pass variable, function not needed Rename previous local variable to class format

[GUI] Tune advanced preferences dialog (6)

0b3d71e

Fix typo causing appveyor error

JuhaSointusalo approved these changes Jul 14, 2018

View reviewed changes

AenBleidd reviewed Jul 16, 2018

View reviewed changes

clientgui/DlgAdvPreferences.h Show resolved Hide resolved

clientgui/DlgAdvPreferencesBase.cpp Show resolved Hide resolved

RichardHaselgrove added this to Review in Client Release 7.14 Aug 16, 2018

AenBleidd approved these changes Sep 8, 2018

View reviewed changes

JuhaSointusalo merged commit c294cf9 into master Sep 9, 2018

JuhaSointusalo mentioned this pull request Sep 9, 2018

Save versus Cancel in config and options type windows.... #2546

Closed

RichardHaselgrove mentioned this pull request Sep 9, 2018

[GUI] Tune simple preferences dialog ... #2687

Closed

RichardHaselgrove moved this from Review to Done in Client Release 7.14 Sep 9, 2018

RichardHaselgrove deleted the RDH_tune_AdvPrefDlg branch September 10, 2018 15:42

RichardHaselgrove mentioned this pull request Sep 10, 2018

[GUI] Tune simple preferences dialog #2689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GUI] Tune advanced preferences dialog #2585

[GUI] Tune advanced preferences dialog #2585

RichardHaselgrove commented Jul 2, 2018

RichardHaselgrove commented Jul 2, 2018

Ageless93 commented Jul 2, 2018

CharlieFenton commented Jul 3, 2018

CharlieFenton commented Jul 3, 2018

TheAspens commented Jul 3, 2018

RichardHaselgrove commented Jul 4, 2018

JuhaSointusalo left a comment

JuhaSointusalo Jul 7, 2018

RichardHaselgrove Jul 12, 2018

JuhaSointusalo Jul 7, 2018

RichardHaselgrove Jul 12, 2018

JuhaSointusalo Jul 7, 2018

davidpanderson Jul 7, 2018

RichardHaselgrove Jul 12, 2018

RichardHaselgrove commented Jul 7, 2018

CharlieFenton commented Jul 7, 2018

JuhaSointusalo commented Jul 8, 2018

RichardHaselgrove commented Jul 12, 2018

JuhaSointusalo commented Jul 12, 2018

JuhaSointusalo commented Jul 12, 2018

JuhaSointusalo commented Jul 12, 2018

RichardHaselgrove commented Jul 12, 2018

JuhaSointusalo commented Jul 14, 2018

JuhaSointusalo commented Sep 8, 2018

AenBleidd left a comment

RichardHaselgrove commented Sep 8, 2018

RichardHaselgrove commented Sep 9, 2018

[GUI] Tune advanced preferences dialog #2585

[GUI] Tune advanced preferences dialog #2585

Conversation

RichardHaselgrove commented Jul 2, 2018

RichardHaselgrove commented Jul 2, 2018

Ageless93 commented Jul 2, 2018

CharlieFenton commented Jul 3, 2018

CharlieFenton commented Jul 3, 2018

TheAspens commented Jul 3, 2018

RichardHaselgrove commented Jul 4, 2018

JuhaSointusalo left a comment

Choose a reason for hiding this comment

JuhaSointusalo Jul 7, 2018

Choose a reason for hiding this comment

RichardHaselgrove Jul 12, 2018

Choose a reason for hiding this comment

JuhaSointusalo Jul 7, 2018

Choose a reason for hiding this comment

RichardHaselgrove Jul 12, 2018

Choose a reason for hiding this comment

JuhaSointusalo Jul 7, 2018

Choose a reason for hiding this comment

davidpanderson Jul 7, 2018

Choose a reason for hiding this comment

RichardHaselgrove Jul 12, 2018

Choose a reason for hiding this comment

RichardHaselgrove commented Jul 7, 2018

CharlieFenton commented Jul 7, 2018

JuhaSointusalo commented Jul 8, 2018

RichardHaselgrove commented Jul 12, 2018

JuhaSointusalo commented Jul 12, 2018

JuhaSointusalo commented Jul 12, 2018

JuhaSointusalo commented Jul 12, 2018

RichardHaselgrove commented Jul 12, 2018

JuhaSointusalo commented Jul 14, 2018

JuhaSointusalo commented Sep 8, 2018

AenBleidd left a comment

Choose a reason for hiding this comment

RichardHaselgrove commented Sep 8, 2018

RichardHaselgrove commented Sep 9, 2018