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

Add a button to open the config file in a text editor #9890

Merged
merged 1 commit into from Apr 10, 2017

Conversation

Projects
None yet
6 participants
@realsolidfox
Contributor

realsolidfox commented Feb 28, 2017

This button was described in issue #9655

I added it to the options dialog

@fanquake fanquake added the GUI label Mar 1, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 1, 2017

Member

Concept ACK

Member

laanwj commented Mar 1, 2017

Concept ACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 1, 2017

Member

Nice.
Concept ACK. Will test soon.

Member

jonasschnelli commented Mar 1, 2017

Nice.
Concept ACK. Will test soon.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 1, 2017

Member
  • I think it should create the file if bitcoin.conf does not exists (right now, there is no reaction if the file does not exists)
  • The buttons should probably aligned/distributed vertically (horizontally it will eat up a lot of space in non en_EN lang)

bildschirmfoto 2017-03-01 um 22 26 46

bildschirmfoto 2017-03-01 um 22 26 32

Member

jonasschnelli commented Mar 1, 2017

  • I think it should create the file if bitcoin.conf does not exists (right now, there is no reaction if the file does not exists)
  • The buttons should probably aligned/distributed vertically (horizontally it will eat up a lot of space in non en_EN lang)

bildschirmfoto 2017-03-01 um 22 26 46

bildschirmfoto 2017-03-01 um 22 26 32

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox
Contributor

realsolidfox commented Mar 1, 2017

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 2, 2017

Contributor

@jonasschnelli Is this what you had in mind for the layout? I wanted to check before pushing

button_demo

Contributor

realsolidfox commented Mar 2, 2017

@jonasschnelli Is this what you had in mind for the layout? I wanted to check before pushing

button_demo

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 2, 2017

Member

@ericshawlinux: I think this looks better and is more flexible.

Member

jonasschnelli commented Mar 2, 2017

@ericshawlinux: I think this looks better and is more flexible.

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 2, 2017

Contributor

I think so too. I amended my commit from the other day

Contributor

realsolidfox commented Mar 2, 2017

I think so too. I amended my commit from the other day

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 2, 2017

Contributor

(I just fixed a mistake I made, I accidentally misplaced a spacer while using Qt-designer. It is back where it belongs now)

Contributor

realsolidfox commented Mar 2, 2017

(I just fixed a mistake I made, I accidentally misplaced a spacer while using Qt-designer. It is back where it belongs now)

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Mar 3, 2017

Member

I wonder if users might get configured that none of their options show up in the config file...? Not really sure how one might improve on this, though.

Member

luke-jr commented Mar 3, 2017

I wonder if users might get configured that none of their options show up in the config file...? Not really sure how one might improve on this, though.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 3, 2017

Member

I wonder if users might get configured that none of their options show up in the config file...? Not really sure how one might improve on this, though.

Yes. We need to watch out here. The configuration levels are complex.

  1. startup arguments (-arg=val)
  2. bitcoin.conf
  3. Qt configuration (GUI)

If a user opens "the" configuration file, he might expect to see all existing configuration values (including the GUI and startup one).

We should probably label the button "open experts configuration file" or similar.

Member

jonasschnelli commented Mar 3, 2017

I wonder if users might get configured that none of their options show up in the config file...? Not really sure how one might improve on this, though.

Yes. We need to watch out here. The configuration levels are complex.

  1. startup arguments (-arg=val)
  2. bitcoin.conf
  3. Qt configuration (GUI)

If a user opens "the" configuration file, he might expect to see all existing configuration values (including the GUI and startup one).

We should probably label the button "open experts configuration file" or similar.

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 3, 2017

Contributor

Well it says "Active command-line options override the above options" so maybe if we put the new button above that, the user will know they are different.

Contributor

realsolidfox commented Mar 3, 2017

Well it says "Active command-line options override the above options" so maybe if we put the new button above that, the user will know they are different.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 3, 2017

Member

An alternative would be to first show an QMessageBox with some infos about how the bitcoin.conf works and after that alert box, open the bitcoin.conf file.

Member

jonasschnelli commented Mar 3, 2017

An alternative would be to first show an QMessageBox with some infos about how the bitcoin.conf works and after that alert box, open the bitcoin.conf file.

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 3, 2017

Contributor

Okay. How does this sound?
"The configuration is used to specify advanced user options less any command-line or Qt options. Any command-line options will override this configuration file."

Contributor

realsolidfox commented Mar 3, 2017

Okay. How does this sound?
"The configuration is used to specify advanced user options less any command-line or Qt options. Any command-line options will override this configuration file."

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 7, 2017

Contributor

I added the information dialog

Contributor

realsolidfox commented Mar 7, 2017

I added the information dialog

Show outdated Hide outdated src/qt/guiutil.cpp Outdated
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 17, 2017

Member

Currently the build is failing on Windows (over Gitian):
https://bitcoin.jonasschnelli.ch/build/53

Otherwise this works fine. Tested on OSX and Ubuntu.

bildschirmfoto 2017-03-17 um 16 09 09

bildschirmfoto 2017-03-17 um 16 09 13

bildschirmfoto 2017-03-17 um 16 17 40

bildschirmfoto 2017-03-17 um 16 17 32

Member

jonasschnelli commented Mar 17, 2017

Currently the build is failing on Windows (over Gitian):
https://bitcoin.jonasschnelli.ch/build/53

Otherwise this works fine. Tested on OSX and Ubuntu.

bildschirmfoto 2017-03-17 um 16 09 09

bildschirmfoto 2017-03-17 um 16 09 13

bildschirmfoto 2017-03-17 um 16 17 40

bildschirmfoto 2017-03-17 um 16 17 32

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 20, 2017

Contributor

okay, I had an issue with git for a few minutes, but I think I fixed it now.

as for the code, I changed the mode from std::fstream::app to std::ios_base::app that builds okay with linux, and I think with windows, since I read the error message you sent me.

Contributor

realsolidfox commented Mar 20, 2017

okay, I had an issue with git for a few minutes, but I think I fixed it now.

as for the code, I changed the mode from std::fstream::app to std::ios_base::app that builds okay with linux, and I think with windows, since I read the error message you sent me.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 20, 2017

Member

Thanks... started again a gitian build: https://bitcoin.jonasschnelli.ch/build/60

Member

jonasschnelli commented Mar 20, 2017

Thanks... started again a gitian build: https://bitcoin.jonasschnelli.ch/build/60

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 20, 2017

Contributor

Still didn't work. I guess I didn't look closely enough at the error before. Now, I think I need to call path.native() instead of path.c_str() based on the constructor declaration.

I will push that soon

Contributor

realsolidfox commented Mar 20, 2017

Still didn't work. I guess I didn't look closely enough at the error before. Now, I think I need to call path.native() instead of path.c_str() based on the constructor declaration.

I will push that soon

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 20, 2017

Member

Please use qstringToBoostPath and boostPathToQString from GUIUtil to convert between QString and boost::filesystem. They avoid UTF-8 issues that will arise when using other methods.

Member

laanwj commented Mar 20, 2017

Please use qstringToBoostPath and boostPathToQString from GUIUtil to convert between QString and boost::filesystem. They avoid UTF-8 issues that will arise when using other methods.

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 20, 2017

Contributor

ok

Contributor

realsolidfox commented Mar 20, 2017

ok

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 20, 2017

Contributor

thanks, I figured it out using boost and didn't need to convert the path there. It should build on windows now

Contributor

realsolidfox commented Mar 20, 2017

thanks, I figured it out using boost and didn't need to convert the path there. It should build on windows now

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 20, 2017

Contributor

@jonasschnelli could you do the gitian build on your site again? I am pretty confident it will work this time after Laan advised me, I changed it to use boost path and not a c string.

Contributor

realsolidfox commented Mar 20, 2017

@jonasschnelli could you do the gitian build on your site again? I am pretty confident it will work this time after Laan advised me, I changed it to use boost path and not a c string.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 20, 2017

Member

Code looks good to me now, although I wonder about error handling: what happens if the config file is not writable?

Member

laanwj commented Mar 20, 2017

Code looks good to me now, although I wonder about error handling: what happens if the config file is not writable?

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 20, 2017

Contributor

When I change the permissions, It asks me how I want to open the file, which doesn't make sense. I can add some logic and show a QMessageBox::critical() dialog if it fails to open the file.

Contributor

realsolidfox commented Mar 20, 2017

When I change the permissions, It asks me how I want to open the file, which doesn't make sense. I can add some logic and show a QMessageBox::critical() dialog if it fails to open the file.

@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 21, 2017

Contributor

Now an error appears when the file cannot be opened.

error2

Contributor

realsolidfox commented Mar 21, 2017

Now an error appears when the file cannot be opened.

error2

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Mar 22, 2017

Member

Testing 541f8d8 on OS X.
Appearance in the GUI:
screen shot 1
Pre opening message:
screen shot 2
Unable to open file error message:
screen shot 4

I'm wondering if we can word the pre-opening message any better. Could we use Core rather than qt, or is that more confusing?

Member

fanquake commented Mar 22, 2017

Testing 541f8d8 on OS X.
Appearance in the GUI:
screen shot 1
Pre opening message:
screen shot 2
Unable to open file error message:
screen shot 4

I'm wondering if we can word the pre-opening message any better. Could we use Core rather than qt, or is that more confusing?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 23, 2017

Member

Maybe replace 'qt options' with 'gui settings'?

Member

MarcoFalke commented Mar 23, 2017

Maybe replace 'qt options' with 'gui settings'?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 23, 2017

Member

Some comments on messages:

  • Agree with @MarcoFalke , "Qt options" should be "GUI settings". Except when directly referring to the widget toolkit we avoid referring to Qt, as that's an implementation detail.
  • "config file" in the error message should be "configuration file" (as the button) or just the filename, "bitcoin.conf". There's no reason to shorten here, and it could make it more difficult for translators.
  • Instead of "less any..." I'd use "except any...". Seems more common English (at least to me as non-native speaker.)
Member

laanwj commented Mar 23, 2017

Some comments on messages:

  • Agree with @MarcoFalke , "Qt options" should be "GUI settings". Except when directly referring to the widget toolkit we avoid referring to Qt, as that's an implementation detail.
  • "config file" in the error message should be "configuration file" (as the button) or just the filename, "bitcoin.conf". There's no reason to shorten here, and it could make it more difficult for translators.
  • Instead of "less any..." I'd use "except any...". Seems more common English (at least to me as non-native speaker.)
@realsolidfox

This comment has been minimized.

Show comment
Hide comment
@realsolidfox

realsolidfox Mar 23, 2017

Contributor

I agree, saying "less any..." is not common, but to make it more descriptive and clear I reworded the whole message the best that I could. How is this?
"The configuration file is used to specify advanced user options which override GUI settings. Additionally, any command-line options will override this configuration file."

Contributor

realsolidfox commented Mar 23, 2017

I agree, saying "less any..." is not common, but to make it more descriptive and clear I reworded the whole message the best that I could. How is this?
"The configuration file is used to specify advanced user options which override GUI settings. Additionally, any command-line options will override this configuration file."

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
Member

jonasschnelli commented Mar 27, 2017

@jonasschnelli jonasschnelli merged commit 9ab9e7d into bitcoin:master Apr 10, 2017

1 check passed

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

jonasschnelli added a commit that referenced this pull request Apr 10, 2017

Merge #9890: Add a button to open the config file in a text editor
9ab9e7d Add a button to open the config file in a text editor (Eric Shaw Jr)

Tree-SHA512: 1d13be9ac788a05a5116dbb3e1136ef65732dc2b5634547860612658109668922c9ea80b77bde4ba5beaa762d54f2a986a6064d4e34e963cdcd3d126a4eced37

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment