refactor: Add OptionsModel getOption/setOption methods #600

1. 
   ryanofsky commented at 7:45 pm on May 19, 2022: contributor

   This is a trivial change which is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It is split off from #602 because it causes a lot of rebase conflicts (any time there is a GUI options change).

   This PR is very small and easy to review ignoring whitespace: https://github.com/bitcoin-core/gui/pull/600/files?w=1

2. refactor: Add OptionsModel getOption/setOption methods

   Easiest to review ignoring whitespace.

   a63b60f02b
3. 
   DrahtBot commented at 8:47 am on May 20, 2022: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #497 (Enable users to configure their monospace font specifically by luke-jr)
   • #440 (Do not require restart if overridden option is modified by hebasto)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

4. hebasto added the label Refactoring on May 20, 2022
5. vasild approved
6. 
   vasild commented at 12:12 pm on May 20, 2022: contributor

   ACK a63b60f02bf7987d0a430496abe524de94f3c8cb
7. in src/qt/optionsmodel.cpp:352 in a63b60f02b

   357+    if(role == Qt::EditRole)
   358+    {
   359+        successful = setOption(OptionID(index.row()), value);
   360+    }
   361+
   362+    Q_EMIT dataChanged(index, index);
   

   ---

   ---

   

   furszy commented at 2:02 pm on May 20, 2022:

   Not for this PR but we actually don’t need to call dataChanged if nothing changed inside setOption (which can happen if the new value is the same as the stored one or if something bad happened internally, like a bad parsing or a impossibility to store the new value).

   ---

   

   promag commented at 10:58 am on May 21, 2022:

   I think it’s fine as is.
8. furszy approved
9. 
   furszy commented at 2:03 pm on May 20, 2022: contributor

   Code review ACK a63b60f0
10. in src/qt/optionsmodel.cpp:601 in a63b60f02b

    821+            settings.setValue("server", value);
    822+            setRestartRequired(true);
    823+        }
    824+        break;
    825+    default:
    826+        break;
    

    ---

    ---

    

    promag commented at 10:58 am on May 21, 2022:

    I wonder if it should set successful = false? Or maybe just drop the default cases and let the compiler complain about unhandled cases?
11. 
    promag commented at 10:58 am on May 21, 2022: contributor

    Code review ACK a63b60f02bf7987d0a430496abe524de94f3c8cb.
12. hebasto merged this on May 22, 2022
13. hebasto closed this on May 22, 2022

    ---

14. 
    jarolrod commented at 11:00 pm on May 22, 2022: member

    post merge ACK https://github.com/bitcoin-core/gui/commit/a63b60f02bf7987d0a430496abe524de94f3c8cb
15. sidhujag referenced this in commit 149fa6e889 on May 28, 2022
16. bitcoin-core locked this on May 22, 2023

Contributors
ryanofsky DrahtBot vasild furszy promag jarolrod

Labels
Refactoring