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

Using a different configuration file name from QT #398

Merged

Conversation

pablomartin4btc
Copy link
Contributor

@pablomartin4btc pablomartin4btc commented Apr 11, 2024

At the moment both QT and QML gui apps are using the same naming convention for configuration files (Bitcoin*.conf), so separating guiconstants.h file from QT, copying it from src/qt to src/qml and updating QAPP_APP_NAME_* constants values will avoid them clashing with each other trying to persist/ read same or different settings (e.g. configuration file for QT on signet will be still named as Bitcoin-Qt-signet.conf, as of today, while QML will start using a separate file named Bitcoin-Qml-signet.conf - before this fix, currently sometimes a user can get a warning on reading incorrect settings or values: QVariant::load: unknown user type with name BitcoinUnits::Unit.).

This could be a temporary fix (? - gui constants file contents has been cleaned up as suggested) so instances from both QT and QML gui apps don't interfere between them during QML development. This change will be transparent for both QT gui app and users.

Sample of a separate QT config file on signet (Bitcoin-Qt-signet.conf).
[General]
DisplayBitcoinUnit=@Variant(\0\0\0\x7f\0\0\0\x13\x42itcoinUnits::Unit\0\0)
MainWindowGeometry=@ByteArray(\x1\xd9\xd0\xcb\0\x3\0\0\0\0\x2\xa3\0\0\x1v\0\0\x5\xb0\0\0\x3^\0\0\x2\xa3\0\0\x1v\0\0\x5\xb0\0\0\x3^\0\0\0\0\0\0\0\0\a\x80\0\0\x2\xa3\0\0\x1v\0\0\x5\xb0\0\0\x3^)
PeersTabBanlistHeaderState=@ByteArray()
PeersTabPeerHeaderState=@ByteArray()
RPCConsoleWindowGeometry=@ByteArray(\x1\xd9\xd0\xcb\0\x3\0\0\0\0\x2q\0\0\x1\x8f\0\0\x5T\0\0\x3<\0\0\x2q\0\0\x1\x8f\0\0\x5T\0\0\x3<\0\0\0\0\0\0\0\0\a\x80\0\0\x2q\0\0\x1\x8f\0\0\x5T\0\0\x3<)
RPCConsoleWindowPeersTabSplitterSizes=@ByteArray(\0\0\0\xff\0\0\0\x1\0\0\0\x2\xff\xff\xff\xff\xff\xff\xff\xff\0\xff\xff\xff\xff\x1\0\0\0\x1\0)
SubFeeFromAmount=false
enable_psbt_controls=false
fCoinControlFeatures=false
fHideTrayIcon=false
fMinimizeOnClose=false
fMinimizeToTray=false
fRestartRequired=false
nSettingsVersion=279900
strDataDir=/home/pablo/.bitcoin
strThirdPartyTxUrls=
Sample of a separate QML config file on signet (Bitcoin-Qml-signet.conf).
[General]
blockclocksize=0.4166666666666667
dark=true
height=665
width=640
x=755
y=339


using namespace std::chrono_literals;

/* A delay between model updates */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these other constants needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

Copy link
Contributor

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. Wonder if we need all of the other constants with the new application?

@pablomartin4btc
Copy link
Contributor Author

pablomartin4btc commented Apr 15, 2024

Concept ACK. Wonder if we need all of the other constants with the new application?

I haven't investigated it yet, the main purpose for this change was to get the config file separate from QT and I clarified at the top that we could do the cleanup later, in case there are features that we don't have/ use in QML at the moment but we'll need them later, but I'm open, we could do the cleanup now and bring the constants back as soon as we need them (if we ever need them).

@johnny9
Copy link
Contributor

johnny9 commented Apr 15, 2024

I tried deleting everything else from that file and the only constant being used is DEFAULT_SPLASHSCREEN. I think we should only keep the constants we are using, I don't think its a good idea to add lines of code that aren't used.

so something like

// Copyright (c) 2011-2024 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_QML_GUICONSTANTS_H
#define BITCOIN_QML_GUICONSTANTS_H

static const bool DEFAULT_SPLASHSCREEN = true;

#define QAPP_ORG_NAME "Bitcoin"
#define QAPP_ORG_DOMAIN "bitcoin.org"
#define QAPP_APP_NAME_DEFAULT "Bitcoin-Qml"
#define QAPP_APP_NAME_TESTNET "Bitcoin-Qml-testnet"
#define QAPP_APP_NAME_SIGNET "Bitcoin-Qml-signet"
#define QAPP_APP_NAME_REGTEST "Bitcoin-Qml-regtest"

#endif // BITCOIN_QML_GUICONSTANTS_H

@pablomartin4btc
Copy link
Contributor Author

DEFAULT_SPLASHSCREEN

I think not even that one, all command line args described in bitcoin.cpp::SetupUIArgs are useless (so far), but regarding the splash-screen (which I mentioned in the wallet intro PR review), @GBKS confirmed, on a designers call a couple of weeks ago, that we are not using that on this project, it's an old fashion approach nowadays.

I'll update the file soon. Thanks!

@pablomartin4btc pablomartin4btc force-pushed the qml-separate-guiconstants-from-qt branch from 1865548 to 54a1f15 Compare April 15, 2024 16:52
@pablomartin4btc
Copy link
Contributor Author

Updates:

Copy link
Contributor

@johnny9 johnny9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 54a1f15

Copy link
Contributor

@D33r-Gee D33r-Gee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 54a1f15 on Ubuntu 22.04

does it need to be rebased?

src/qml/guiconstants.h Outdated Show resolved Hide resolved
Separating guiconstants.h file from QT, copying it from src/qt to src/qml,
removing all unused constants so far and updating QAPP_APP_NAME_* constants
values so both QT and QML gui apps don't clash with each other trying to
persist same or different settings (e.g. configuration file for QT on signet
will be still named as Bitcoin-Qt-signet.conf, as of today, while QML will
start using a separate file named BitcoinCore-App-signet.conf).

This could be a temporary fix so instances from both QT and QML gui apps
don't interfere between them during QML development. This change will be
transparent for both QT app and users.

Co-authored-by: Johnny <[email protected]>
@pablomartin4btc pablomartin4btc force-pushed the qml-separate-guiconstants-from-qt branch from 54a1f15 to 5be1e52 Compare April 23, 2024 21:33
@pablomartin4btc
Copy link
Contributor Author

Updates:

  • Addressed @hebasto's feedback on file naming convention.

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tack 5be1e52

@hebasto hebasto merged commit 085805f into bitcoin-core:main Apr 24, 2024
8 of 9 checks passed
hebasto added a commit to hebasto/gui-qml that referenced this pull request May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants