Qt: Reflect correct dbcache value in options panel. #11915

valentinewallace commented at 7:50 AM on December 16, 2017: none

Fixes: #11788 Shows correct value according to command line override or value in bitcoin.conf.

valentinewallace force-pushed on Dec 16, 2017

laanwj commented at 7:58 AM on December 16, 2017: member

I don't think this is correct. The command line option should override the GUI setting with respect to the value used, but it should not set/change the GUI setting. Command line arguments shouldn't stick, which I think is what this does?

(reason is, for example: to be able to run with -dbcache=10000 temporarily, then quit, then run without -dbcache and go back to the normal setting)

fanquake added the label GUI on Dec 16, 2017

fanquake deleted a comment on Dec 16, 2017

valentinewallace commented at 11:15 PM on December 17, 2017: none

@laanwj Sorry I'm not sure I understand. When I run the GUI with this code, if I run with -dbcache=10000 it will display 10000 in the options menu. If I then quit and run without the flag, I see the either number in bitcoin.conf or the default in the options menu.

Are you saying when you run without the flag a second time it still displays the last flag value used?

laanwj commented at 1:17 PM on December 19, 2017: member

Yes. This:

settings.setValue("nDatabaseCache", (qint64)gArgs.GetArg("-dbcache", nDefaultDbCache));

Writes the value from the command line / bitcoin.conf to the QSettings object persistently. That's not how it's supposed to work.

jonasschnelli commented at 6:52 PM on December 19, 2017: contributor

To fix #11788, I would propose to show the "underlaying" dbcache value (set via bitcoin.conf or -dbcache=<value>) in the GUI settings pannel...

but only, if the value has not been overwritten by the QT settings ( (!settings.contains("nDatabaseCache")).

Also, only write the GUI set database cache to QSettings when...

Either nDatabaseCache is set
Or the new value is not equal to the underlaying -dbcache value.

valentinewallace commented at 1:14 AM on December 21, 2017: none

@jonasschnelli

but only, if the value has not been overwritten by the QT settings ((!settings.contains("nDatabaseCache")).

Currently, due to init in optionsmodel, QSettings will always contain nDatabaseCache, it'll just contain the default if the user doesn't give it a different value through the GUI. However, this could be altered so it doesn't get set in settings unless the user sets it.

Intuitively, my takeaway from your comment is that the GUI settings panel should display:

QSettings value if set by the user
else flag value if set
else bitcoin.conf if set
else default value

My one reservation is that it makes more sense to me for the GUI to display the currently effective settings no matter what (a la the second comment in this thread). If we're going with showing the effective settings, I changed my code and I'll update the PR, otherwise will keep chugging along :)

jonasschnelli commented at 7:14 AM on December 22, 2017: contributor

Oh. Your right... Current code:

if (!settings.contains("nDatabaseCache"))
        settings.setValue("nDatabaseCache", (qint64)nDefaultDbCache);

This seems suboptimal. After executing this, you have no way to find out if the user has set a value or if we just have copied over the default from nDefaultDbCache.

I think it should only write a value to nDatabaseCache if it was set by the user and if it was different as the default. Also, if nDatabaseCache is set, there could be a different color indicating that the value has been "overwritten".

But I agree, it should always show the current active database cache size.

valentinewallace commented at 10:39 PM on December 22, 2017: none

That line you mentioned^ was there before I touched the file so I put it back in the spirit of making as few changes as possible to solve the problem (maybe I should've done the commits differently?), but I agree it does seem suboptimal.

I think it should only write a value to nDatabaseCache if it was set by the user and if it was different as the default.

Might it make sense to write a value even if the user sets it to the default value? Otherwise the bitcoin.conf value will override it even if the value the user wants in QSettings happens to be the default.

Re: only writing a value to nDatabaseCache if user-set-- one tradeoff is that it makes the code for initing the DatabaseCache value in QSettings different from how all the other QSettings values are initialized which is currently very uniform, adding a bit of complexity.

However, I'll look into making that change and the color-coding tmrw :)

valentinewallace commented at 4:27 AM on January 1, 2018: none

Okay, now we only write to QSettings if the user sets a non-default value!

in src/qt/optionsmodel.cpp:414 in 63bf237c08 outdated

 405 | @@ -404,7 +406,7 @@ bool OptionsModel::setData(const QModelIndex & index, const QVariant & value, in
 406 |              Q_EMIT coinControlFeaturesChanged(fCoinControlFeatures);
 407 |              break;
 408 |          case DatabaseCache:
 409 | -            if (settings.value("nDatabaseCache") != value) {
 410 | +            if (settings.value("nDatabaseCache") != value && QVariant::fromValue(gArgs.GetArg("-dbcache", nDefaultDbCache)) != value && value != QVariant::fromValue(nDefaultDbCache)) {

jonasschnelli commented at 10:40 AM on February 12, 2018:

What if someone has dbcache=2000 in bitcoin.conf and then tries to reduce the dbcache via GUI back to 450?

valentinewallace commented at 8:50 PM on February 14, 2018:

Currently that wouldn't do anything because we don't write to QSettings if the user sets the GUI dbcache value to the default. Should we switch such that we write to QSettings no matter what value the user sets?

jonasschnelli assigned jonasschnelli on Mar 19, 2018

jonasschnelli commented at 7:12 PM on April 10, 2018: contributor

Needs rebase

valentinewallace force-pushed on Apr 16, 2018

valentinewallace commented at 9:36 PM on April 16, 2018: none

@jonasschnelli rebased!

DrahtBot added the label Needs rebase on Jun 11, 2018

in src/qt/optionsmodel.cpp:93 in 2c71ba28e5 outdated

  87 | @@ -88,9 +88,10 @@ void OptionsModel::Init(bool resetSettings)
  88 |      // by command-line and show this in the UI.
  89 |  
  90 |      // Main
  91 | -    if (!settings.contains("nDatabaseCache"))
  92 | -        settings.setValue("nDatabaseCache", (qint64)nDefaultDbCache);
  93 | -    if (!m_node.softSetArg("-dbcache", settings.value("nDatabaseCache").toString().toStdString()))
  94 | +   std::string nDbCacheSettings = settings.contains("nDatabaseCache") ?
  95 | +     settings.value("nDatabaseCache").toString().toStdString() :
  96 | +     std::to_string(nDefaultDbCache);

Empact commented at 1:47 AM on October 9, 2018:

whitespace issue here, you can use git diff -U0 HEAD~3.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v to fix: https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/README.md#clang-format-diffpy

valentinewallace commented at 6:32 PM on October 22, 2018:

good one, thanks!

in src/qt/optionsmodel.cpp:286 in 2c71ba28e5 outdated

 282 | @@ -282,7 +283,11 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
 283 |          case CoinControlFeatures:
 284 |              return fCoinControlFeatures;
 285 |          case DatabaseCache:
 286 | -            return settings.value("nDatabaseCache");
 287 | +            if (!gArgs.GetArg("-dbcache", "").empty())

Empact commented at 1:50 AM on October 9, 2018:

IsArgSet could be more explicit

valentinewallace commented at 6:33 PM on October 22, 2018:

fixed :)

in src/qt/optionsmodel.cpp:304 in 2c71ba28e5 outdated

 282 | @@ -282,7 +283,11 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
 283 |          case CoinControlFeatures:
 284 |              return fCoinControlFeatures;
 285 |          case DatabaseCache:
 286 | -            return settings.value("nDatabaseCache");
 287 | +            if (!gArgs.GetArg("-dbcache", "").empty())
 288 | +                return (qint64)gArgs.GetArg("-dbcache", nDefaultDbCache);
 289 | +            if (settings.contains("nDatabaseCache"))
 290 | +                return settings.value("nDatabaseCache");
 291 | +            return QVariant::fromValue(nDefaultDbCache);

Empact commented at 1:53 AM on October 9, 2018:

Seems you can return the default directly, the QVariant will be constructed from it.

valentinewallace commented at 6:33 PM on October 22, 2018:

I seem to be getting this error when I try to do that:

deepinscreenshot_select-area_20181021212834

Empact commented at 6:53 PM on October 23, 2018:

How about casting instead as above?

valentinewallace commented at 9:11 PM on October 24, 2018:

that works, and makes the code prettier! fixed :)

in src/qt/optionsmodel.cpp:301 in 2c71ba28e5 outdated

 282 | @@ -282,7 +283,11 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
 283 |          case CoinControlFeatures:
 284 |              return fCoinControlFeatures;
 285 |          case DatabaseCache:
 286 | -            return settings.value("nDatabaseCache");
 287 | +            if (!gArgs.GetArg("-dbcache", "").empty())
 288 | +                return (qint64)gArgs.GetArg("-dbcache", nDefaultDbCache);

Empact commented at 1:54 AM on October 9, 2018:

Why is this cast necessary?

valentinewallace commented at 6:35 PM on October 22, 2018:

Hm, for the code to compile? Am I missing something? here's what happens when I remove the cast:

deepinscreenshot_select-area_20181021215649

fanquake commented at 5:51 AM on October 20, 2018: member

@valentinewallace Are you still interested in working on this? If so, could you address any nits & rebase. Otherwise we could close & label "Up for Grabs" etc.

MarcoFalke added the label Up for grabs on Oct 20, 2018

valentinewallace commented at 4:11 AM on October 21, 2018: none

@fanquake yes, sorry about that. I was planning to update it tomorrow if that's okay!

valentinewallace force-pushed on Oct 22, 2018

DrahtBot removed the label Needs rebase on Oct 22, 2018

valentinewallace force-pushed on Oct 24, 2018

Qt: Reflect and persist correct dbcache value from options panel.

Resulting behavior:
- If the `dbcache` flag is set, the value is displayed but not persisted.
- If the user sets a value in the gui, it is persisted to QSettings.
- Else the default is displayed (but not persisted).

b38659b7cf

valentinewallace force-pushed on Oct 24, 2018

DrahtBot commented at 8:14 PM on November 13, 2018: member

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

Conflicts

No conflicts as of last run.

dongcarl commented at 4:46 AM on February 22, 2019: member

No longer needed as #11788 is closed.

dongcarl closed this on Feb 22, 2019

dongcarl removed the label Up for grabs on Feb 22, 2019

DrahtBot locked this on Dec 16, 2021