Persist "mask values" in gui #655

pull jadijadi wants to merge 1 commits into bitcoin-core:master from jadijadi:jadi-issue652-persist-mask changing 4 files +14 −0
  1. jadijadi commented at 11:26 AM on August 25, 2022: contributor

    fixes #652

    <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. hernanmarino approved
  3. hernanmarino commented at 2:48 PM on September 10, 2022: contributor

    tested ACK

  4. furszy changes_requested
  5. furszy commented at 3:22 PM on September 10, 2022: member

    Need to add an option inside the OptionsModel and use setOption to store/update it. Not call directly the backend function, it breaks the layers division.

    Essentially, OptionsModel class is the interface from the GUI to the node's configuration data structure. In other words, GUI widgets/models uses the OptionsModel to access/modifiy the node settings.

  6. jadijadi commented at 12:59 PM on September 12, 2022: contributor

    Need to add an option inside the OptionsModel and use setOption to store/update it. Not call directly the backend function, it breaks the layers division.

    Thanks for the important note. I'm traveling this week with limited access to my dev machine / internet but will try to provide a fix per your suggestion soon. Thanks again.

  7. ryanofsky commented at 5:42 PM on September 12, 2022: contributor

    Need to add an option inside the OptionsModel and use setOption to store/update it. Not call directly the backend function, it breaks the layers division.

    Also, instead of using Node::getPersistentSetting and Node::updateRwSetting methods, this should probably use QSettings::value and QSettings::setValue methods to access the setting. If you look at OptionsModel you can see that it uses QSettings to store GUI preferences. The Node methods are only used to store settings which are bitcoind command line or configuration file options (and -privacy is not a bitcoind option).

  8. Persist "mask values" in gui
    fixes https://github.com/bitcoin-core/gui/issues/652
    9fde0bae91
  9. jadijadi force-pushed on Sep 22, 2022
  10. jadijadi commented at 7:45 AM on September 23, 2022: contributor

    tested ACK

    Thanks for the tests & ACK. I've done some changes based on the comments from @ryanofsky and @furszy . Can you please have another look? Thank you in advance.

  11. jadijadi commented at 7:46 AM on September 23, 2022: contributor

    Need to add an option inside the OptionsModel and use setOption to store/update it. Not call directly the backend function, it breaks the layers division.

    Also, instead of using Node::getPersistentSetting and Node::updateRwSetting methods, this should probably use QSettings::value and QSettings::setValue methods to access the setting. If you look at OptionsModel you can see that it uses QSettings to store GUI preferences. The Node methods are only used to store settings which are bitcoind command line or configuration file options (and -privacy is not a bitcoind option).

    thanks @ryanofsky & @furszy . I've updated the PR based your comment; via OptionsModel.

  12. in src/qt/optionsmodel.cpp:57 in 9fde0bae91 
      53 | @@ -54,6 +54,7 @@ static const char* SettingName(OptionsModel::OptionID option)
  54 |      case OptionsModel::ProxyPortTor: return "onion";
  55 |      case OptionsModel::ProxyUseTor: return "onion";
  56 |      case OptionsModel::Language: return "lang";
  57 | +    case OptionsModel::Privacy: return "privacy";
    ryanofsky commented at 5:00 AM on September 24, 2022:

    In commit "Persist "mask values" in gui" (9fde0bae916f73b73fcdbeff6469dba72c4be212)

    This change to SettingName function doesn't do anything and should be reverted. SettingName maps OptionModel enum values to bitcoin.conf / command line option names, and there is no "-privacy" option, so this wouldn't do the right thing even if the case was ever used.

    If you want to use OptionsModel to read and write this setting, instead of changing the SettingName function, you should change the OptionsModel::getOption() function by adding:

        case Privacy:
        return settings.value("privacy");

    And the OptionsModel::setOption() function by adding:

        case Privacy:
        settings.setValue("privacy", value.toBool());
        break;

    You could grep CoinControlFeatures or EnablePSBTControls to see how another similar GUI settings implemented in OptionsModel.

  13. in src/qt/overviewpage.cpp:182 in 9fde0bae91 
     177 | @@ -176,6 +178,9 @@ void OverviewPage::handleTransactionClicked(const QModelIndex &index)
 178 |  
 179 |  void OverviewPage::setPrivacy(bool privacy)
 180 |  {
 181 | +    QSettings settings;
 182 | +    settings.setValue("privacy", privacy);
    ryanofsky commented at 5:22 AM on September 24, 2022:

    In commit "Persist "mask values" in gui" (9fde0bae916f73b73fcdbeff6469dba72c4be212)

    I don't think this is too important, but I'm not sure if I agree with furszy's comment that this setting needs to be part of OptionsModel. It seems nice to make it part of OptionsModel but also reasonable to not make it part of OptionsModel and just read/write it with QSettings directly.

    But probably it would be better to pick one approach or the other. Either don't add the setting to OptionsModel and use QSettings to read and write. Or do add the setting to OptionsModel and use OptionsModel to read and write. Otherwise you wind up causing inconsistent access to the setting across layers that furszy was trying to avoid #655#pullrequestreview-1103143874

  14. hebasto renamed this:
    qt - Persist "mask values" in gui
    Persist "mask values" in gui
    on Oct 25, 2022
  15. hebasto added the label UX on Oct 26, 2022
  16. hernanmarino commented at 4:36 PM on December 6, 2022: contributor

    re ACK 9fde0bae916f73b73fcdbeff6469dba72c4be212

  17. hebasto cross-referenced this on Feb 2, 2023 from issue Persist Mask Values option by achow101
  18. hebasto commented at 2:11 PM on February 7, 2023: member

    It looks like #701 is being actively reviewed recently. Let's combine all discussions there.

  19. DrahtBot commented at 2:11 PM on February 7, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hernanmarino

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  20. hebasto closed this on Feb 7, 2023
  21. bitcoin-core locked this on Feb 7, 2024
Contributors
jadijadihernanmarinofurszyryanofskyhebastoDrahtBot
Labels
UX
Linked (view graph)
#652 Persist "mask values" to prevent shoulder surfing#701 Persist Mask Values option
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-27 02:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me