qt: allow user to choose data directory #2670
Conversation
|
I'm going to take a look next week, can you post a screen until I do :)? |
|
OK selection Warning when directory already exists, user can still click OK Invalid or unreachable path Too little free space on device |
|
Changed Gb -> GB, checking now happens in separate thread to prevent blocking the GUI thread. |
|
Just some small comments:
|
|
> Do you allow OK even when low disk space was detected, looks like OK is available there. Yes. The user can choose OK in all cases where it's possible to create the directory. They may plain on cleaning up the drive immediately (ie, deleting one blueray image should be enough to make space). In the end it's their own responsibility. > Do you intend to show this even for existing installations or just new ones Only for new installations (when the default data directory doesn't exist). Otherwise it will confuse users into making a new data directory and they will wonder where their block chain / wallet will have gone. > Do we want to place a clickable URI to official Bitcoin site here or anywhere else in the client? I think that would be a good addition. I'm not sure why, unless it links to an explanation about data directories. But feel free to add more flair to the window later. I'm just focusing on the functionality for now. > Perhaps display none for available space when case "Invalid or unreachable path" Yeah... or N/A > Perhaps we could also add a clickable button for launching -choosedatadir into the options dialog, when I finish my work there? I don't want to support changing the data directory while the client is running. If the user wants to move the data directory they can do so themselves while the client is not running, and the "choose data directory" dialog will automatically pop up on next run as the software notices that the data directory does not exist. |
src/util.cpp
Outdated
There was a problem hiding this comment.
I guess Gavin would love to see some unit test for this :-P. Btw. I also thought about such a thing a few days ago ^^.
There was a problem hiding this comment.
I didn't like adding this function, but I didn't see another way to do this (well ok it's possible to assign to mapArgs directly, as it is exported outside util.cpp, this is done in rpcmining.cpp for example... but that wouldn't be better would it).
SetSoftArg is not good enough as the directory selected in the dialog should override everything, even the data directory passed on the command line, otherwise it'd be confusing. Another solution would be to not show the dialog at all when the datadir is overridden on the command line. Hmm that may be better.
|
ObUIshedpaint (feel free to ignore): The first "Warning:" is perhaps a bit intense and should probably be more reserved for things with risk of irreparable harm (encrypting wallets, deleting wallet directories, sending a 1000 BTC fee, etc.). Using space is just a fact of normal software operation and is nothing to be concerned about... unless you don't have enough— so I think it's okay in a low/insufficient space case... |
|
@gmaxwell: fixed that, all the warnings was indeed a bit over the top |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ff78dd81170c15f7fed00f803dde109283e78c72 for binaries and test log. |
|
"official bitcoin site"?! I suspect you mean "original satoshi client site"...
|
|
Just a question. When you write 'GB', do you really mean 'GB' - i.e. 10^9, or do you instead mean 'GiB' - i.e. 2^30 ? On May 20, 2013, at 7:41 AM, Wladimir J. van der Laan wrote:
|
|
I really hate that GiB stuff, for me all units are 1024 based but I would never use GiB, KiB or such nonsense ^^. |
|
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/55605706a21e2325720c989ad690f83aa2934f78 for binaries and test log. This could happen for one of several reasons:
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ |
|
Ugh, I'm trying to find out the OS path separator using make_preferred() for a message but this doesn't exist on the ancient boost used by pulltester. |
|
Should be solved now. |
|
@laanwj I also think we should drop support for ancient Boost version. I never understood, why it is a problem to do so anyway. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e024910520ab3868240beebfacec04de15c929ca for binaries and test log. |
|
I understand that many hate it. However, your insistence on casually using units which never had any standards organization bless them in any way leaves you perpetuating a needless ambiguity, and in conflict with SI, ANSI, NIST, ISO, IEC, ITC, IETF, BSA, and just about any accredited standards organization one might care to name. All of whom have issued clear guidance that (e.g.) GB always refers to 10^9, and GiB always refers to 2^30. On May 21, 2013, at 12:07 AM, Philip Kaufmann wrote:
|
|
As someone who has been using computers since 1979, I can confirm that KB
|
|
Can you take the units discussion elsewhere please? These kinds of |
|
Perhaps have two choices when the client starts up for the first time, the first to download the block chain with all the defaults, and the second being to change the data directory or whatever. This will alleviate the technicality of it for non-technical people, and make the first dialogue screen more welcoming. Something maybe like: Start Block Chain | Change Directory |
|
@Suffice As a follow-up, maybe there can be some menu option "Move datadir", which requires a restart immediately afterwards (or not, but that's definitely a lot more work). I think that's independent from this pullreq, which already improves 99% of use cases. |
|
@Suffice yeah maybe... but I do want to make sure people see the warning about downloading 10Gb, as one of the reasons for this is that people don't get freaked out later when they see their harddisk being filled up. In any case they already accept the default by clicking "OK" immediately. On a fresh install, it will initially show the default data directory. @sipa Could be useful option. But I feel better about that once block chain dir != wallet dir. No problem with moving the block chain, it can be redownloaded if the user makes a mistake, but as moving implies a delete I don't really want to "move" the wallet.dat. Copy at most. Let people that know what they're doing do this manually for now. |
|
@laanwj ACK, separating datadir from wallets must happen first. |
|
@Suffice this is better I think. It makes clear that the initially selected directory is the default, and requires an extra radio button click to change it to prevent it from being changed cluelessly. And it still shows what the default data directory is (though disabled), and whether there is enough space there. |
|
I don't know anymore. . Pulling this off in best way possible is tricky. This is how most program installs look: Perhaps something like this: I noticed that the block chain is currently stored in the AppData directory on Windows, will that change? It would be nice if the wallet auto purged parts of the block chain that are no longer needed, and perhaps requests parts of the chain that it needs again, like in the event of importing addresses. (Tricky) New clients could start from the near end of this chain, as they won't likely need the whole chain. |
|
I noticed that the block chain is currently stored in the AppData directory on Windows, will that change? After this pull it will be possible to change that, at least on new installs, that's the point of it. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/e8d6ef18ac4d38ca93827f739aa560345c5fcff1 for binaries and test log. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/aaa6b5e87b69f6cbcca3d1fc430d8bc9f73ac609 for binaries and test log. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4c7685061d2437b9c3a1c61e490dd673d9a7ed6e for binaries and test log. |
src/qt/intro.cpp
Outdated
There was a problem hiding this comment.
Can you add a small comment that this is 10GB and below 1GB currently :)?
There was a problem hiding this comment.
Indeed could be clarified. But as such comments tend to go out of date,
making it clear in the code through multiplying 10 with a constant GB may
be better.
There was a problem hiding this comment.
Yeah, so perhaps just switch the 2 and use GB_BYTES * 10 then :).
|
It behaves a little weird, let me explain:
Edit: Sorry, I didn't have the recent rebase changes in my local branch... will test again! Edit 2: Behaviour is the same, I would await, that @laanwj Did you take a look at this comments yet? |
src/qt/intro.cpp
Outdated
There was a problem hiding this comment.
Can you explain to me what this is for?
There was a problem hiding this comment.
When you enter a path that does not exist, it has to find the first parent
path up the tree that does exist to be able to determine the free space
on that file system. Boost will not accept non existing paths.
There was a problem hiding this comment.
You've got some nasty bug in this code part.
Step 1:
de_DE
Step 2 (because of a QSetting):
de
Step 3 (still from the QSetting):
de
lang = de and after this line it is just empty, which leads to the default language getting activated!
There was a problem hiding this comment.
Thanks for testing and reporting, I'll take a look.
There was a problem hiding this comment.
After your recent changes the thing here is like this:
- lang_territory = de_DE
- lang_territory = de (as that is my stored QSetting)
- lang_territory = de (as I didn't pass
-lang)
So lang = de before doing the truncate() and is empty afterwards. So you pass an empty variable lang into the further calls, where it should be de and de (via lang_territory) where it should be de_DE, right? It seems to work okay, but I'm not sure if this is intended :).
There was a problem hiding this comment.
That's how it always worked -- if lang_territory contains a string such as "de_DE", lang will be "de" and both will be loaded. If lang_territory contains a string such as "de", lang will be "" and just "de" will be loaded. It will indeed pass an empty string, but as it cannot find a translation with an empty name that's a no-op.
|
Rebased, and language problem solved (the QTranslators were getting freed). |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/6aad2cacef1af45185773622548ba74c755bbe79 for binaries and test log. |
|
@Diapolo does it still show the strange behavior on Windows? If so, any idea what causes that? |
|
Sugestion: Can you make the info/error messages selectable and show the beam-cursor? Translations are working again! Anyway, I'll report what is happening, my default datadir is: Start with -choosedatadir and default selected: Change from default to custom: Change path into Change path into I think the default detected path |
There was a problem hiding this comment.
Can you change these style-sheets to color: #800000; to be CSS standard-conformant?
There was a problem hiding this comment.
I'm pretty sure this is not required by the standard (stack overflow agrees, see top answer here http://stackoverflow.com/questions/11939595/leaving-out-the-last-semicolon-of-a-css-block ). Can you quote me the part of the standard where it says that the last declaration needs a ; separator?
There was a problem hiding this comment.
Seems you are correct here, I checked via: http://jigsaw.w3.org/css-validator/#validate_by_input and both are correct, with and without ;.
This adds an introduction screen that is shown when the client is first started in which the user can choose a data directory. It is also possible to force the intro screen to appear using command line argument `-choosedatadir`. The user is warned that the client will download and store 10Gb of data. The intro screen shows how much space is available on the device that contains the chosen directory and warns if this is less than the 10Gb. To make it possible to translate the introduction dialog, the initialization sequence is changed so that translations are loaded before the data directory. This has the by-effect that it is no longer possible to specify a language in bitcoin.conf inside the data directory.
|
Should be solved now. |
|
The directory things and disk-space stuff is fixed, great :). |
|
Feel free to play with the layout a bit... |
|
GUI can be improved later :-). ACK |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/be77b637fcf5983286382a9b9677fb97b026abe2 for binaries and test log. |
There was a problem hiding this comment.
Perhaps you can update this to use TestNet() from chainparams.h, which you need to include here then? Otherwise I can do this after it's merged :).
Edit: We don't need chainparams.h as it's coming from init.h alreay.
There was a problem hiding this comment.
Are you sure that will work? The reason for looking for testnet arg here
directly is that it is called before Init.
There was a problem hiding this comment.
I tried it and it doesn't work, as we are before init here. I think it would be nice, if we could achieve to use TestNet() with re-ordering the code, but my try doesn't yet do this work ^^. See #2788
Anyway, I would suggest you merge this pull and we can look into this afterwards.
|
@laanwj Can we perhaps merge this now :)? |
|
ACK - seems to work fine. |
qt: allow user to choose data directory
9 participants
This adds an introduction screen that is shown when the client is first started in which the user can choose a data directory.
It is also possible to force the intro screen to appear using command line argument
-choosedatadir.The data directory is remembered in the QSettings.
The user is warned that the client will download and store 10Gb of data. The intro screen shows how much space is available on the device that contains the chosen directory and warns if this is less than the 10Gb.
To make it possible to translate the introduction dialog, the initialization sequence is changed so that translations are loaded before the data directory. This has the by-effect that it is no longer possible to specify a language in bitcoin.conf inside the data directory.
Please help testing!