Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qt: Add "Blocksdir" to Debug window #14374

Merged
merged 2 commits into from Oct 18, 2018

Conversation

Projects
None yet
9 participants
@hebasto
Copy link
Member

commented Oct 2, 2018

To get the current blocksdir is valuable for debug purposes after
merging #12653.

screenshot from 2018-10-02 23-16-52

@fanquake fanquake added the GUI label Oct 2, 2018

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

utACK 16d79b064c095accc8a3c1decdf54cdd03554564 (see issue below)

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

Coverage Change (pull 14374) Reference (master)
Lines +0.0331 % 87.0471 %
Functions +0.1853 % 84.1130 %
Branches +0.0171 % 51.5403 %
@promag
Copy link
Member

left a comment

Concept ACK. Could have a way to tell if it's the default blocks directory or user supplied.

@@ -177,6 +177,11 @@ QString ClientModel::dataDir() const
return GUIUtil::boostPathToQString(GetDataDir());
}

QString ClientModel::blocksDir() const
{
return GUIUtil::boostPathToQString(GetBlocksDir());

This comment has been minimized.

Copy link
@promag

promag Oct 5, 2018

Member

Should not call GetBlockDir but get it from interfaces::Node? cc @ryanofsky

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 8, 2018

Contributor

In the long run, promag is right and adding an interfaces::Node method would be better, but for now calling GetBlocksDir() directly is more consistent with existing code. The goal of adding the Node and Wallet interfaces in #10244 was to get rid of accesses to non-GUI global variables from GUI code. But to make the change more minimal, I exempted gArgs and chainparams variables, and instead added some glue code in #10102 to let each process just keep its own copies of these variables. As a result, it's fine to call functions like GetBlocksDir and GetDataDir from GUI code that access them.

@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

@promag and/or a tooltip that explains this location can be customized using -blocksdir

Agree we need to clarify if GetBlockDir should be avoided in favor of adding a method to interfaces::Node.

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

@Sjors @promag
Thank you for your reviews.

Should not call GetBlockDir but get it from interfaces::Node?

Agree we need to clarify if GetBlockDir should be avoided in favor of adding a method to interfaces::Node.

Besides GetBlocksDir the ClientModel calls GetDataDir, GetStartupTime and GetTimeMillis as well. All are the thread-safe utilities not from the node code. IMO, using the GetBlocksDir is a safe way.

Nevertheless, I agree with all of you: @ryanofsky's review will be much appreciated.

Add "Blocksdir" to Debug window
To get the current blocksdir is valuable for debug purposes after
merging #12653.

@hebasto hebasto force-pushed the hebasto:20181002-debugwindow-blocksdir branch Oct 5, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2018

a tooltip that explains this location can be customized using -blocksdir

Done:

screenshot from 2018-10-06 00-15-15

@Sjors
Copy link
Member

left a comment

tACK bde3946 modulo the tooltip html.

src/qt/forms/debugwindow.ui Outdated
@@ -127,6 +127,9 @@
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
</property>
<property name="toolTip">
<string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;To specify a non-default location of the data directory use the '&amp;#8209;datadir' option.&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>

This comment has been minimized.

Copy link
@Sjors

Sjors Oct 8, 2018

Member

What's with the &lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;? This probably makes the tooltip hard to translate.

This comment has been minimized.

Copy link
@hebasto

hebasto Oct 8, 2018

Author Member

It uses HTML code &#8209; for the non-breaking hyphen character. Otherwise, the tooltip can looks ugly.
This LOC has been generated by Qt Creator and, therefore, the translation should be treated in a correct way. Or did I miss something?

This comment has been minimized.

Copy link
@Sjors

Sjors Oct 8, 2018

Member

The &amp;#8209; bit seems fine, but I was talking about the html etc prefix. I think most developers use a text editor instead of the Qt Creator tools. Perhaps @laanwj knows if this works with the translation system?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 8, 2018

Contributor

Can you just use the utf8-encoding of the chr(8209) character? ? Presumably then you wouldn't need the html stuff.

@ryanofsky
Copy link
Contributor

left a comment

utACK bde394669ff5dc666d8b8ad1c0e74244a427eec0

src/qt/forms/debugwindow.ui Outdated
@@ -127,6 +127,9 @@
<property name="cursor">
<cursorShape>IBeamCursor</cursorShape>
</property>
<property name="toolTip">
<string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;To specify a non-default location of the data directory use the '&amp;#8209;datadir' option.&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Oct 8, 2018

Contributor

Can you just use the utf8-encoding of the chr(8209) character? ? Presumably then you wouldn't need the html stuff.

@hebasto hebasto force-pushed the hebasto:20181002-debugwindow-blocksdir branch to 2ab9140 Oct 8, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2018

@Sjors

This probably makes the tooltip hard to translate.

You are right. I've missed this warning Avoid HTML in translation strings

@ryanofsky
Thank you for your review.

Tooltips are fixed. Please re-review.

@ryanofsky
Copy link
Contributor

left a comment

utACK 2ab9140 but I think it would be better if you just literally wrote the nonbreaking hyphen in the XML as a utf8-encoded character (as suggested previously):

-          <string>To specify a non-default location of the data directory use the '%1' option.</string>
+          <string>To specify a non-default location of the data directory use the '‑datadir' option.</string>
-          <string>To specify a non-default location of the blocks directory use the '%1' option.</string>
+          <string>To specify a non-default location of the blocks directory use the '‑blocksdir' option.</string>
@Sjors

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Actually I like the %1 approach better. It prevents translators from accidentally breaking the - or worse, translating the actual command.

utACK 2ab9140

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Tested a bit.
I think we should only display this when the blocksdir is not the default.

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Concept ACK

@laanwj laanwj merged commit 2ab9140 into bitcoin:master Oct 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 18, 2018

Merge #14374: qt: Add "Blocksdir" to Debug window
2ab9140 Add tooltips for both datadir and blocksdir (Hennadii Stepanov)
3045704 Add "Blocksdir" to Debug window (Hennadii Stepanov)

Pull request description:

  To get the current `blocksdir` is valuable for debug purposes after
  merging #12653.

  ![screenshot from 2018-10-02 23-16-52](https://user-images.githubusercontent.com/32963518/46374770-2ef6f580-c69a-11e8-85c2-44a49fa36b28.png)

Tree-SHA512: a93f2c00ee19cf6acb499d3bd9bccf4be8ef01c53c44d917ad401aa4797db02cbccb71a9c24e05262ea09345e15f9299381367fdc6951f21dd3788a4a58d2132

@hebasto hebasto deleted the hebasto:20181002-debugwindow-blocksdir branch Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.