Only write options after onboarding #284

johnny9 · 2023-03-09T03:48:15Z

hebasto · 2023-05-30T09:17:33Z

Needs rebase.

johnny9 · 2023-05-31T13:39:40Z

update from 45e3262 to 08be85b:

rebased over main

thebagmaster · 2023-06-04T02:01:42Z

ACK 08be85b
I tested these on Ubuntu 22.04.2 and it does what it says on the tin.
Understood that we may need to buffer the settings writing to file in on-boarding; especially if the storage location is not set and may change.
Maybe we should do our best to save what we can of what the user configured?
Are there situations where we buffer settings and different settings that we save immediately?

hebasto · 2023-08-31T22:41:41Z

Please rebase.

https://discord.com/channels/903125802726596648/903126039084015638/1146922061449666602

johnny9 · 2023-09-01T00:48:39Z

Update from 08be85b to 51df97d

rebased with main

jarolrod

Concept ACK

jarolrod · 2023-05-26T21:25:17Z

src/qml/options_model.cpp

@@ -15,8 +15,9 @@

 #include <cassert>

-OptionsQmlModel::OptionsQmlModel(interfaces::Node& node)
+OptionsQmlModel::OptionsQmlModel(interfaces::Node& node, bool is_onboarded)


potentially, a bool indicating if we're onboarded or not could be valuable to other models. Since right now we don't have such a use case, this is fine.

D33r-Gee · 2023-12-11T18:23:22Z

tACK on WSL Ubuntu 22.04 on signet

Works as expected, the settings file gets created after the onboarding has finished.

settings.json:

{ "listen": true, "natpmp": true, "prune": 2861 }

Do we want the “Listen” and “natpmp” options to be true by default or was it just for testing this PR?

D33r-Gee · 2023-12-11T20:04:49Z

tACK on Android armv7 everything worked as expected

The settings.json was created after the onboarding with the following values:

{ "prune": 2861 }

D33r-Gee

ACK 51df97d

pablomartin4btc

Concept ACK, I'll test soon. It looks that there are some unaddressed feedback.

pablomartin4btc

tACK 51df97d

Tested in both master(/ main) and this PRs branch: changing all settings touched by this branch's code change and verify the changes were saved accordingly.

Things I noticed during testing:

In main:

If the user doesn't change anything setting.json file will look like:

{
    "prune": 1907
}

If the user disables any settings setting.json file will look like:

{
    "prune": 0
    "listen": false,
    "natpmp": false,
    "server": false,
    "upnp": false

}

In branch johnny9:write_after_onboard:

If the user doesn't change anything setting.json file will look like:

{
    "dbcache": 16000,
    "listen": true,
    "natpmp": true,
    "prune": 1907,
    "server": true,
    "upnp": true
}

If the user disables any settings setting.json file will look like:

{
    "dbcache": 16000,
}

After the user makes any changes on the settings mentioned above next time the user starts `bitcoin-qt` up again it looks like the UI doesn't match the configuration file (`settings.json`), is this expected?

Using main:

Using this branch:

…core#284

johnny9 · 2024-03-29T03:31:54Z

Update 51df97d to 61f8e6c

Rebased with master
Initialized all uninitialized OptionsQmlModel properties

johnny9 · 2024-03-29T14:44:59Z

In branch johnny9:write_after_onboard:

If the user doesn't change anything setting.json file will look like:
{
    "dbcache": 16000,
    "listen": true,
    "natpmp": true,
    "prune": 1907,
    "server": true,
    "upnp": true
}
If the user disables any settings setting.json file will look like:
{
    "dbcache": 16000,
}
After the user makes any changes on the settings mentioned above next time the user starts bitcoin-qt up again it looks like the UI doesn't match the configuration file (settings.json), is this expected?

@pablomartin4btc I wasn't able to reproduce the output you were seeing during onboarding but I think I fixed the settings not applying at next startup with initializing the values in the constructor.

D33r-Gee · 2024-03-29T16:56:34Z

tACK 61f8e6c

On Ubuntu 22.04 and Android (armv7)

Works as expected, settings.json shows defaults:

{
    "listen": true,
    "prune": 1907
}

pablomartin4btc

tACK 61f8e6c

It behaves as expected.

dbcache setting is not persisted anymore ("dbcache": 16000,), is this correct?

johnny9 · 2024-04-05T11:06:31Z

dbcache setting is not persisted anymore ("dbcache": 16000,), is this correct?

It should only persist if you set it to a value other than nDefaultDbCache (450)

pablomartin4btc · 2024-04-05T16:01:04Z

dbcache setting is not persisted anymore ("dbcache": 16000,), is this correct?

It should only persist if you set it to a value other than nDefaultDbCache (450)

Perfect, it works.

johnny9 · 2024-04-10T03:49:03Z

Update from 61f8e6c to 4f557b0

rebased with main

johnny9 · 2024-04-10T11:41:39Z

Update from 4f557b0 to 9e56907

fixed build issue with previous rebase conflict (forgot to remove <<<<<<<)

D33r-Gee

tACK 9e56907

lgtm, works as expected on both Ubuntu 22.04 and Android (armv7)

played around with different settings and they registered properly both in the settings.json and in the debug.log

pablomartin4btc

re-ACK 9e56907

MarnixCroes

9e56907
writing of the settings works, but it always additionally creates a settings.json.bak file?

pablomartin4btc · 2024-04-11T19:05:52Z

src/qml/models/options_model.cpp

@@ -8,6 +8,7 @@
 #include <common/settings.h>
 #include <common/system.h>
 #include <interfaces/node.h>
+#include <mapport.h>
 #include <qt/guiconstants.h>
 #include <qt/guiutil.h>
 #include <qt/optionsmodel.h>


Do we need this?

Suggested change

#include <qt/optionsmodel.h>

It is needed for PruneMiBtoGB i believe

pablomartin4btc · 2024-05-06T22:14:02Z

src/qml/models/options_model.cpp

+    }
+    m_onboarded = true;
+}


nit: accidentally removed the last line break?

Suggested change

}

}

already has a newline character at the end of the file. don't need an additional blank line.

D33r-Gee

re ACK 9e56907 on Ubuntu 22.04 works as expected

johnny9 marked this pull request as ready for review March 9, 2023 12:46

hebasto added the UX Designers' opinions are required label Apr 17, 2023

jarolrod added this to the v1.0 milestone May 29, 2023

hebasto added the Needs rebase label May 30, 2023

johnny9 force-pushed the write_after_onboard branch from 45e3262 to 08be85b Compare May 31, 2023 13:38

jarolrod removed the Needs rebase label May 31, 2023

hebasto modified the milestones: v1.0, v1.1 Jun 6, 2023

johnny9 force-pushed the write_after_onboard branch from 08be85b to 51df97d Compare September 1, 2023 00:47

jarolrod reviewed Sep 8, 2023

View reviewed changes

D33r-Gee reviewed Mar 6, 2024

View reviewed changes

pablomartin4btc reviewed Mar 6, 2024

View reviewed changes

D33r-Gee mentioned this pull request Mar 8, 2024

DRAFT: Custom Datadir Wiring #390

Draft

pablomartin4btc reviewed Mar 13, 2024

View reviewed changes

D33r-Gee pushed a commit to D33r-Gee/gui-qml that referenced this pull request Mar 22, 2024

qml: integrated Johny9's Only write options after onboarding bitcoin-…

590d0c2

…core#284

johnny9 force-pushed the write_after_onboard branch from 51df97d to 61f8e6c Compare March 29, 2024 03:27

pablomartin4btc approved these changes Apr 5, 2024

View reviewed changes

qml: Set OptionQmlModel initial property values

c0ebff2

johnny9 force-pushed the write_after_onboard branch from 61f8e6c to 4f557b0 Compare April 10, 2024 03:48

qml: only write options after onboarding

9e56907

johnny9 force-pushed the write_after_onboard branch from 4f557b0 to 9e56907 Compare April 10, 2024 04:13

D33r-Gee reviewed Apr 10, 2024

View reviewed changes

pablomartin4btc approved these changes Apr 10, 2024

View reviewed changes

MarnixCroes reviewed Apr 11, 2024

View reviewed changes

pablomartin4btc reviewed Apr 11, 2024

View reviewed changes

D33r-Gee mentioned this pull request May 6, 2024

UI Only Custom Datadir Display #397

Open

pablomartin4btc reviewed May 6, 2024

View reviewed changes

johnny9 requested a review from jarolrod May 8, 2024 14:12

D33r-Gee reviewed May 14, 2024

View reviewed changes

hebasto merged commit 84d388d into bitcoin-core:main May 14, 2024
8 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only write options after onboarding #284

Only write options after onboarding #284

johnny9 commented Mar 9, 2023 •

edited

hebasto commented May 30, 2023

johnny9 commented May 31, 2023

thebagmaster commented Jun 4, 2023 •

edited

hebasto commented Aug 31, 2023

johnny9 commented Sep 1, 2023

jarolrod left a comment

jarolrod May 26, 2023

D33r-Gee commented Dec 11, 2023

D33r-Gee commented Dec 11, 2023

D33r-Gee left a comment

pablomartin4btc left a comment

pablomartin4btc left a comment

johnny9 commented Mar 29, 2024

johnny9 commented Mar 29, 2024

D33r-Gee commented Mar 29, 2024

pablomartin4btc left a comment

johnny9 commented Apr 5, 2024

pablomartin4btc commented Apr 5, 2024

johnny9 commented Apr 10, 2024

johnny9 commented Apr 10, 2024 •

edited

D33r-Gee left a comment

pablomartin4btc left a comment

MarnixCroes left a comment

pablomartin4btc Apr 11, 2024

johnny9 May 3, 2024

pablomartin4btc May 6, 2024

johnny9 May 8, 2024 •

edited

D33r-Gee left a comment

Only write options after onboarding #284

Only write options after onboarding #284

Conversation

johnny9 commented Mar 9, 2023 • edited

hebasto commented May 30, 2023

johnny9 commented May 31, 2023

thebagmaster commented Jun 4, 2023 • edited

hebasto commented Aug 31, 2023

johnny9 commented Sep 1, 2023

jarolrod left a comment

Choose a reason for hiding this comment

jarolrod May 26, 2023

Choose a reason for hiding this comment

D33r-Gee commented Dec 11, 2023

D33r-Gee commented Dec 11, 2023

D33r-Gee left a comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

johnny9 commented Mar 29, 2024

johnny9 commented Mar 29, 2024

D33r-Gee commented Mar 29, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

johnny9 commented Apr 5, 2024

pablomartin4btc commented Apr 5, 2024

johnny9 commented Apr 10, 2024

johnny9 commented Apr 10, 2024 • edited

D33r-Gee left a comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

pablomartin4btc Apr 11, 2024

Choose a reason for hiding this comment

johnny9 May 3, 2024

Choose a reason for hiding this comment

pablomartin4btc May 6, 2024

Choose a reason for hiding this comment

johnny9 May 8, 2024 • edited

Choose a reason for hiding this comment

D33r-Gee left a comment

Choose a reason for hiding this comment

johnny9 commented Mar 9, 2023 •

edited

thebagmaster commented Jun 4, 2023 •

edited

johnny9 commented Apr 10, 2024 •

edited

johnny9 May 8, 2024 •

edited