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

Bugfix: Use unique autostart filenames on Linux for testnet/regtest #7045

Merged
merged 1 commit into from Nov 24, 2015

Conversation

@luke-jr
Copy link
Member

luke-jr commented Nov 17, 2015

Currently, mainnet/testnet/regtest use the same file for their autostarts.

@jonasschnelli
Copy link
Member

jonasschnelli commented Nov 18, 2015

utACK

@MarcoFalke
Copy link
Member

MarcoFalke commented Nov 18, 2015

utACK

@@ -354,7 +354,10 @@ boost::filesystem::path static GetAutostartDir()

boost::filesystem::path static GetAutostartFilePath()
{
return GetAutostartDir() / "bitcoin.desktop";
std::string chain = ChainNameFromCommandLine();
if (chain == CBaseChainParams::MAIN)

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 18, 2015

Member

I don't think we need this special case.
What's wrong with bitcoin-main.lnk?

This comment has been minimized.

Copy link
@luke-jr

luke-jr Nov 18, 2015

Author Member

It's not the same filename being used presently. Thus, users with 0.11 autostarting would see the checkbox uncheck itself, and checking it would result in trying to autostart twice...

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 19, 2015

Member

Hm, true. Yeah.

@laanwj
Copy link
Member

laanwj commented Nov 18, 2015

Concept ACK

std::string chain = ChainNameFromCommandLine();
if (chain == CBaseChainParams::MAIN)
return GetAutostartDir() / "bitcoin.desktop";
return GetAutostartDir() / strprintf("bitcoin-%s.lnk", chain);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Nov 19, 2015

Member

nit: s/strprintf("bitcoin-%s.lnk", chain) / "bitcoin-"+chain+".lnk"? But maybe a matter of taste.

This comment has been minimized.

Copy link
@paveljanik

paveljanik Nov 19, 2015

Contributor

Depends on from where you come :-) But of course this should be C++ ;-)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 19, 2015

Member

I used this + "" + syntax a lot in earlier days but I don't think this is suitable for a project like bitcoin: The "+something+" syntax usually produces unreadable and nasty diffs when changed.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Nov 19, 2015

Member

No strong opinion. Just wanted to say that strprintf in a non-std function brought in over a tiny headers only library (tinyformat) and the + " " + syntax will very likely result in simpler and faster machine code.

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 19, 2015

Member

This is only called once per invocation of the application at most, so let's not micro-optimize.

@laanwj laanwj merged commit 2aa49ce into bitcoin:master Nov 24, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Nov 24, 2015
2aa49ce Bugfix: Use unique autostart filenames on Linux for testnet/regtest (Luke Dashjr)
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Nov 30, 2015
2aa49ce Bugfix: Use unique autostart filenames on Linux for testnet/regtest (Luke Dashjr)
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 8, 2015
2aa49ce Bugfix: Use unique autostart filenames on Linux for testnet/regtest (Luke Dashjr)
laanwj added a commit that referenced this pull request Jul 18, 2019
ae311bc Fix autostart filenames on Linux (Hennadii Stepanov)

Pull request description:

  Currently, on master the `bitcoin-test.lnk` and `bitcoin-regtest.lnk` files do not work as autostart application `.desktop` files.

  This PR fixes it.

  Refs:
  - #7045
  - [Autostart Of Applications During Startup](https://standards.freedesktop.org/autostart-spec/autostart-spec-latest.html)

ACKs for top commit:
  promag:
    utACK ae311bc, weird why extension `.lnk` was used in #7045.
  laanwj:
    Code review ACK ae311bc

Tree-SHA512: 210cc346600d52b0a262c81ed5f258365a3cea2e5522f4b5f4798fd3b54f45ed82aba68eefae59a6b6f1d8e4d00221476c23bdffc038f16f2f45c1acc837f522
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.