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

Already on GitHub? Sign in to your account

Qt GUI #521

Merged
merged 338 commits into from Sep 26, 2011

Conversation

Projects
None yet
10 participants
Owner

laanwj commented Sep 18, 2011

This pull requests merges in the Qt GUI ( https://github.com/laanwj/bitcoin-qt/blob/master/README.rst ). This introduces a qmake-based build system completely orthogonal to the current build system and UI code. All qt-related source files and resources are added under src/qt: 135 of the 154 "Files Changed" are additions.

Changes to core code have been kept to a minimum and are generally guarded by #ifdef or clearly UI-specific. Outline of changes to core:

  • Add InitMessage calls to initialization sequence, for splash screen. These do nothing on Qt or GUI-less builds
  • #ifdefs for QT GUI specific changes: there is no default or preferred receiving address, and no visible addresses are generated without the users intervention
  • Keep track of maximum block count of peers (GetNumBlocksOfPeers), for visual indication of progress
  • Translation functions and printf take std::string instead of char*, to allow _ to safely redirect to Qt translation service
  • Add GetUnconfirmedBalance() call to CWallet, to be able to show non-confirmed balance in home screen
  • rpc.(cpp|h) renamed to bitcoinrpc.(cpp|h) to satisfy qmake build system on Windows

Discussion on forum: https://bitcointalk.org/index.php?topic=44330.msg532087
and https://bitcointalk.org/index.php?topic=15276.0

laanwj and others added some commits Jun 11, 2011

@laanwj laanwj Add tooltips, make "amount" entry consistent in Options dialog 5e1fedd
@laanwj laanwj Somewhat confident now, tested on GNOME+KDE, with all types of transa…
…ctions. Next step is integration into bitcoin tree.
5813089
@laanwj laanwj move back to original directory structure ba4081c
@laanwj laanwj add build instructions to doc 0304f4a
@laanwj laanwj use stylized icon by bitboy 37f793c
@laanwj laanwj remove wallet updating debug output c428d9e
@laanwj laanwj add the bitcoin (MIT?) license 0424613
@laanwj laanwj update bitcoin core to git ce14894 18cf214
@laanwj laanwj prepare internationalization; rename project to bitcoin-qt ab2fe68
@laanwj laanwj add svg version of icon, so that it can be scaled to bigger sizes later 3ac5aa4
@laanwj laanwj replace icons d066e25
@laanwj laanwj new icons -- test 92ab03a
@laanwj laanwj Status column reorganization 249300a
@laanwj laanwj Address book: select action (edit/select) based on context e83474f
@laanwj laanwj Internationalization -- initial step, make _ return a std::string to …
…prevent memory leaks
39cf857
@laanwj laanwj Internationalization -- conversion of strings from bitcoin core 6315130
@laanwj laanwj link to -lcrypto as well f15df6b
@laanwj laanwj Add berkelydb version warning 5363cb0
@laanwj laanwj Make status column narrow (icon only, details on tooltip) a790ec5
@laanwj laanwj add connection meter b1ef1b2
@laanwj laanwj icons test cf450e1
@laanwj laanwj transaction status icons 58557b5
@laanwj laanwj add attribution for icons aec8763
@laanwj laanwj show connection meter "full" only at 10+ connections 8c69f1f
@laanwj laanwj better icons for confirmations 89c94b5
@laanwj laanwj add svg sources for icons e61cfaf
@laanwj laanwj remove unused icons 553af2f
@laanwj laanwj add license for md2k7's icons e4347a4
@laanwj laanwj fix issue #3 (dark theme compat) aa52972
@laanwj laanwj remove commented code, use // for one-line comments and comments insi…
…de functions
0f3981b
@laanwj laanwj Prevent notification balloon-spam on initial block download, const-co…
…rrectness in client model
7df70c0
@laanwj laanwj Use explicit resource initialization, apparently needed on some platf…
…orms
45c4a0b
@laanwj laanwj add configure and receive icon 245ab4d
@laanwj laanwj update bitcoin core from git (eeac872) 0eeb4f5
@laanwj laanwj On initial block chain download, show a progress bar 6cab663
@laanwj laanwj initial block download spans total blocks minus threshold 0f9ee79
mark fixes for mac build 3c7ebae
@laanwj laanwj Merge pull request #5 from mjmvisser/master
mac fixes
6795927
@laanwj laanwj number of confirmations is no longer magic value 18b99e3
@laanwj laanwj introduce bitcoin amount field with split amount/decimals, to protect… f193c57
@laanwj laanwj Fix some padding and focus issues with the new BitcoinAmountWidget 84114e3
@laanwj laanwj Add comment to tooltip that only sending addresses can be deleted 54e903a
@laanwj laanwj fix issue #7 50d08dc
@laanwj laanwj when going to decimals field using ./, select it all, so that entry s…
…tarts from scratch instead of appending to previous value
c92fc34
@laanwj laanwj highlight default address f5927f5
@laanwj laanwj Allow changing default address (fixes issue #6) b9e8098
@laanwj laanwj clarify text 5f280ff
@laanwj laanwj use #ifdef QT_UI to distinguish Qt UI instead of hardcoded #if 0 47c6215
@laanwj laanwj experiment with internationalization (nl), unbreak build (externui.h-…
…>qtui.h)
daaee73
@laanwj laanwj use buttonbox for options dialog 6665c24
@laanwj laanwj finish nl translation 40951d8
@laanwj laanwj Call "initial download" "synchronizing with network" instead c88e14f
@laanwj laanwj allow adding address to address book in send dialog 38deedc
@laanwj laanwj Improve look/usablity of send coins dialog a404b15
@laanwj laanwj compile fixes by Unthinkingbit 0030c1b
@laanwj laanwj update dutch translation 42e950a
@laanwj laanwj fix typo in dutch translation cae5264
@laanwj laanwj reduce spacing between "Add to address book as" and the text field d99f5a4
@laanwj laanwj update core to d0d8017 (CWallet class) e8ef3da
@laanwj laanwj Change transaction table:
- Split "Description" column into "Type" and "Address", to make sorting easier (and facilitate filtering in the future)
- Merged "credit" and "debit" columns into one "amount" column that can be black (positive) or red (negative)
34fa178

msva and others added some commits Aug 28, 2011

@msva @laanwj msva + laanwj add russian translation and add unicode compatibility (merges pull re…
…quest #20)
3f0816e
@laanwj laanwj Wallet encryption part 2: ask passphrase when needed, add menu options b7bcaf9
@laanwj laanwj comments and readme update 6c85cbe
@laanwj laanwj Merge branch 'master' of https://github.com/bitcoin/bitcoin f43f46c
@laanwj laanwj Merge branch 'master' of https://github.com/bitcoin/bitcoin
Conflicts:
	src/main.cpp
7a15d4f
@laanwj laanwj update to work with new lock system, add protocol.* to build system c5aa1b1
@laanwj laanwj bitcoin-qt cannot be used as command line rpc client b2d1129
@laanwj laanwj support USE_UPNP setting 0aca857
@laanwj laanwj Edited README.rst via GitHub 4785e6d
@laanwj laanwj Merge branch 'master' of https://github.com/bitcoin/bitcoin 0a70a3f
@laanwj laanwj Merge branch 'master' of github.com:laanwj/bitcoin-qt 69e8763
@laanwj laanwj (k)ubuntu 10.04+ notification support (based on @zwierzak his code) cf9195c
@Matoking @laanwj Matoking + laanwj Pull request #21: windows fixes/cleanup by Matoking 94723e2
@laanwj laanwj fix the build (moved code use 'this' instead of 'window') f077d1a
@laanwj laanwj Merge branch 'master' of https://github.com/bitcoin/bitcoin
Conflicts:
	.gitignore
a0d2f9a
@laanwj laanwj http -> https 9b9e2f1
@Matoking @laanwj Matoking + laanwj The synchronization progress bar now compares the amount of total blo…
…cks to amount of blocks downloaded at application start-up. Could be probably implemented better.
78b3bf5
@laanwj laanwj clarify function signature (GetNumBlocksOfPeers) and use number of 'f…
…rozen' blocks as initial value for number of peer blocks
d33cc2b
@p2k p2k Some Mac OS X specific things
* Added application icon for Mac OS X * Added instructions for compiling
under Mac OS X * Added Portfile for compiling miniupnpc with MacPorts
2c1fd3c
@laanwj laanwj Merge pull request #22 from p2k/master
Some improvements for Mac OS X
1837644
@laanwj laanwj remove transparency effect and windows-specific code for now, it's no…
…t working as supposed
83312d7
@flower1024 @laanwj flower1024 + laanwj make German translation up-to-date c1e6672
@laanwj laanwj Merge branch 'master' of https://github.com/bitcoin/bitcoin 5dd7318
@laanwj laanwj move qt-specific scripts to qt-specific directory in scripts/ 3c66913
@laanwj laanwj assure that base bitcoind and bitcoin still build e122e42
@laanwj laanwj move current qt specific readme to doc/, restore original README.md 0465c41
Owner

sipa commented Sep 19, 2011

ACK on the changes to core. I haven't reviewed the UI code itself.

tcatm commented Sep 19, 2011

This seems to break the wxgui (error in AppInit()). Either that should be fixed or wxgui removed IMHO.

Owner

laanwj commented Sep 19, 2011

Didn't notice it yet. I'll fix that. The changes should not affect the wx GUI.

Owner

laanwj commented Sep 19, 2011

tcatm: I'm unable to reproduce this (on Linux). The Wx still works as it did before. Can you get a traceback for the error?

tcatm commented Sep 19, 2011

sorry, my fault: I forgot to run make clean and thus part of bitcoin was usind bdb 4.7 and bdb 4.8

Contributor

gavinandresen commented Sep 19, 2011

ACK, I did a quick review and setup a qt build environment. Looks good.

@TheBlueMatt TheBlueMatt commented on the diff Sep 21, 2011

bitcoin-qt.pro
+# DEFINES += SSL
+CONFIG += no_include_pwd
+
+# for boost 1.37, add -mt to the boost libraries
+LIBS += -lssl -lcrypto -ldb_cxx
+unix:!macx:LIBS += -lboost_system -lboost_filesystem -lboost_program_options -lboost_thread
+macx:LIBS += -lboost_system-mt -lboost_filesystem-mt -lboost_program_options-mt -lboost_thread-mt
+macx:DEFINES += __WXMAC_OSX__ MSG_NOSIGNAL=0 BOOST_FILESYSTEM_VERSION=3
+windows:LIBS += -lboost_system-mgw44-mt-1_43 -lboost_filesystem-mgw44-mt-1_43 -lboost_program_options-mgw44-mt-1_43 -lboost_thread-mgw44-mt-1_43 -lws2_32 -lgdi32
+windows:DEFINES += __WXMSW__
+windows:RC_FILE = src/qt/res/bitcoin-qt.rc
+
+# use: qmake "USE_UPNP=1"
+# miniupnpc (http://miniupnp.free.fr/files/) must be installed
+count(USE_UPNP, 1) {
+ message(Building with UPNP support)
@TheBlueMatt

TheBlueMatt Sep 21, 2011

Contributor

USE_UPNP should be tri-state. ie USE_UPNP=0 is different than USE_UPNP undefined.

@laanwj

laanwj Sep 21, 2011

Owner

It is tri-state:

qmake USE_UPNP=
-> miniupnpc not linked / compiled in

qmake USE_UPNP=0
-> miniupnpc is compiled in, but disabled by default (-llibupnpc is linked
in, USE_UPNP=0 is set)

qmake USE_UPNP=1
-> miniupnpc is compiled in, and enabled by default (-llibupnpc is linked
in, USE_UPNP=0 is set)

(as documented in doc/readme-qt.rst)

I've just tested this out, and it works as expected.

@laanwj

laanwj Sep 21, 2011

Owner

(uh, the last one should say "...USE_UPNP=1 is set")

@TheBlueMatt

TheBlueMatt Sep 21, 2011

Contributor

mmm, I guess I just don't know how to read qmake files, or am blind...probably both. Just wanted to check to make sure.

@TheBlueMatt TheBlueMatt commented on the diff Sep 21, 2011

contrib/miniupnpc/Portfile
@@ -0,0 +1,43 @@
+# -*- coding: utf-8; mode: tcl; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- vim:fenc=utf-8:filetype=tcl:et:sw=4:ts=4:sts=4
@TheBlueMatt

TheBlueMatt Sep 21, 2011

Contributor

Im not against adding this file if it is needed, but can I ask why it is?

@laanwj

laanwj Sep 21, 2011

Owner

It was added by @p2k to improve MacOSX support, I suppose it's some kind of
auto-build file.

@TheBlueMatt

TheBlueMatt Sep 21, 2011

Contributor

Its a macport port file for the latest miniupnpc. That's fine but I'm not sure about putting it in the bitcoin repo. It should probably just be submitted upstream to macports and not be put on bitcoin.

@p2k

p2k Sep 21, 2011

Contributor

Yes, this is a temporary solution and I will remove this file as soon as macports fixes the issue.

@TheBlueMatt

TheBlueMatt Sep 21, 2011

Contributor

Meh, still not sure about putting it in bitcoin, but I guess it doesn't matter.

@p2k

p2k Sep 21, 2011

Contributor

I'll try to send it to macports right now and remove it in the very minute they accept it, ok?

EDIT: You can monitor the process here: https://trac.macports.org/ticket/31354

@TheBlueMatt TheBlueMatt commented on the diff Sep 21, 2011

doc/readme-qt.rst
+
+- Notification on incoming / outgoing transactions (compatible with FreeDesktop and other desktop notification schemes)
+
+- General interface improvements: Splash screen, tabbed interface
+
+- Overview page with current balance, unconfirmed balance, and such
+
+- Better transaction list with status icons, real-time filtering and a context menu
+
+- Asks for confirmation before sending coins, for your own safety
+
+- CSV export of transactions and address book (for Excel bookkeeping)
+
+- Shows alternative icon when connected to testnet, so you never accidentally send real coins during testing
+
+- Shows a progress bar on initial block download, so that you don't have to wonder how many blocks it needs to download to be up to date
@TheBlueMatt

TheBlueMatt Sep 21, 2011

Contributor

H ow safely Is this done wrt people modifying nodes to report incorrect block counts?

@laanwj

laanwj Sep 21, 2011

Owner

They could report more blocks than they actually have (not less, as the
maximum is taken of peers and a hardcoded minimum bound
nTotalBlocksEstimate is taken into account). This could mess up the
progress bar, but apart from that influences nothing. It's just a visual
indicator.

H ow safely Is this done wrt people modifying nodes to report incorrect
block counts?

Reply to this email directly or view it on GitHub:
https://github.com/bitcoin/bitcoin/pull/521/files#r134664

@TheBlueMatt

TheBlueMatt Sep 21, 2011

Contributor

Would it not make sense to try to drop outliers just to keep people from complaining if they get odd progress bars?

@laanwj

laanwj Sep 21, 2011

Owner

Maybe, but that's extra complexity which could introduce bugs in itself. I
didn't think that'd be worth adding the extra complexity to the core. But it
could keep track of the numbers reported by the last N nodes connected to,
then report the median, for example...

Would it not make sense to try to drop outliers just to keep people from

complaining if they get odd progress bars?

Reply to this email directly or view it on GitHub:
https://github.com/bitcoin/bitcoin/pull/521/files#r134725

@TheBlueMatt

TheBlueMatt Sep 21, 2011

Contributor

Maybe its just me, but id feel better if it at least took some effort to spoof, even if not much.

Contributor

TheBlueMatt commented Sep 21, 2011

I'm assuming no one cares about adding non mit pictures to bitcoin, but I might as well bring up the issue.

Contributor

TheBlueMatt commented Sep 21, 2011

Also, it might just be me but I really hate having two sets of translations, is it not possible to redefine some translation functions and make a script to convert the format of the files?

Owner

laanwj commented Sep 21, 2011

This is exactly what I did (see scripts/qt/extract_strings_qt.py). When
running with Qt UI, the _ function is redirected to Qt. The Wx translations
are not used by the Qt UI.

Owner

laanwj commented Sep 21, 2011

Feel free to add or replace your own images if you think that's a concern...

Contributor

TheBlueMatt commented Sep 21, 2011

Well look at me not paying attention, but I also meant for qt files to use _ so that translation programs will pick up strings from qtui as well (or did you do that too, im on my phone and the browser lags out if I try to load the diff tab).
Also I'm assuming you have some strings specific to qtui already, how were those handled in the existing translations for qt?

Contributor

TheBlueMatt commented Sep 21, 2011

In URI handling, why does it not parse the message field?

Owner

laanwj commented Sep 21, 2011

Nothing special has to be done for that; Qt uses tr() instead of _, and
recognizes the qtui-specific strings (which are indeed a lot) in
lupdate/lrelease, the qt translation tools, automatically.

It does not pick up _(), that's why the python script exists. It converts
the strings in _() to QT_TRANSLATE_NOOP strings which are recognized.

This means that qt translator can be used to translate the strings in the
core as well as the qtui-specific strings in one go.

On Wed, Sep 21, 2011 at 9:26 PM, Matt Corallo <
reply@reply.github.com>wrote:

Well look at me not paying attention, but I also meant for qt files to use
_ so that translation programs will pick up strings from qtui as well (or
did you do that too, im on my phone and the browser lags out if I try to
load the diff tab).
Also I'm assuming you have some strings specific to qtui already, how were
those handled in the existing translations for qt?

Reply to this email directly or view it on GitHub:
#521 (comment)

Owner

laanwj commented Sep 21, 2011

Huh I don't see anything about a message field in the URL handling? What's it supposed to parse, and do with it?

Contributor

TheBlueMatt commented Sep 21, 2011

re:images I dont have a problem with it but I thought id bring it up to make sure no one else had any problems with it.
Re:translations, it just bugs me that half the translations are in po format and the other half are in qt. That might cause issues later on if there are conflicting translations used by bitcoind. My question was whether or not it would be possible to modify the qt-generated code to use the_() function (which is mapped to tr) so that translation software would pick up the strings that qt needs as well as wx and bitcoind ones. That way all the translations would be in one place.

Contributor

TheBlueMatt commented Sep 21, 2011

See https://en.bitcoin.it/wiki/URI_Scheme
(ignore lukes tonal exponent crap)

Owner

laanwj commented Sep 21, 2011

Eventually all the translations should be in qt format (ts), not the other
way around, IMO. The Qt translation system is more advanced (context
sensitive, can take into account multiples, shows the GUI while translating
etc).

The po files could be generated from the ts files, for use by bitcoind, and
wouldn't even need to be in git. This would need another script but not a
very complex one.

Owner

laanwj commented Sep 21, 2011

What would you want to do with the message? Display it in a popup? Currently, the "send coins" tab is opened and the amount, address and label is simply added to the recipients list when a URL is drag&dropped into the client.

Owner

laanwj commented Sep 21, 2011

BTW bitcoind doesn't use translation at all; I guess it'd need to link against libgettext to do that. Currently, without ui, _ is implemented as this (util.h):

#if !defined(QT_GUI) && !defined(GUI)
inline const char* _(const char* psz)
{
    return psz;
}
#endif

If this is kept the same, this means the po files can simply be removed when the wx UI goes away.

tcatm commented Sep 21, 2011

RE: message in bitcoin uri

It should be stored as a comment for that transaction. Actually, I'd like to have a editable comment field for every transaction (outgoing and incoming) so one could label individual transactions on a single address. I think the wallet and RPC already supports comments.

Owner

laanwj commented Sep 21, 2011

Is that true? I haven't noticed it. I intended to add such functionality. A long time ago I sent a message on the mailing list about adding metadata to transactions and address book addresses, but no one ever replied, so I guessed it was not practical or useless.

Anyway, we can add this later. It's a good idea in any case.

Contributor

p2k commented Sep 21, 2011

laanwj: Just a side note: I've written a python script that reads ts files and outputs unicode string tables for use in mac or iphone applications. I think it won't take me long to modify it so it outputs po files.

Owner

laanwj commented Sep 21, 2011

p2k: nice, if we decide to add gettext-based translation to bitcoind that will certainly come in handy

Contributor

TheBlueMatt commented Sep 21, 2011

Meh, I dont care if all the translations are po or qt, but I would think it would be better to have them all together instead of split between two files.

Owner

laanwj commented Sep 21, 2011

As I said, unless gettext-based translation is added to bitcoind, we can
simply remove the .po files when the wx UI is deprecated. That would solve
the problem wouldn't it?

Contributor

TheBlueMatt commented Sep 21, 2011

yes., long as existing translations are ported over.

Owner

laanwj commented Sep 21, 2011

Yes, that could be done, though that'd only work for the strings in the
bitcoin core. Most of the strings in the UI itself would remain untranslated
as they're different from before.

@gavinandresen gavinandresen added a commit that referenced this pull request Sep 26, 2011

@gavinandresen gavinandresen Merge pull request #521 from laanwj/qt
Qt GUI
f7f2a36

@gavinandresen gavinandresen merged commit f7f2a36 into bitcoin:master Sep 26, 2011

@coblee coblee pushed a commit to litecoin-project/litecoin that referenced this pull request Jul 17, 2012

@gavinandresen gavinandresen Merge pull request #521 from laanwj/qt
Qt GUI
34cd269

@ptschip ptschip pushed a commit to ptschip/bitcoin that referenced this pull request Aug 2, 2017

@gandrewstone gandrewstone Merge pull request #521 from marlengit/gui/coin_freeze_cltv
[Review] More UI fixes
50a6c32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment