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

Backport Bitcoin Qt/Gui changes up to 0.14.x part 1 #1614

Merged
merged 11 commits into from
Sep 7, 2017

Conversation

codablock
Copy link

@codablock codablock commented Sep 5, 2017

This is the first PR in a series of PRs to backport all Qt/Gui changes of Bitcoin up to 0.14.x.

The PRs were chosen by running:

git log --merges bitcoin/0.14 ^v0.12.2.x -- src/qt > backport-qt.txt

And then cherry picked into my local branch with:

git cherry-pick -m1 <hash-of-bitcoin-pr>

I tried to stick with Qt/Gui related PRs only, but sometimes it was required to pull in additional commits to resolve merge conflicts and compilation errors.
Also, my method of gathering the required PRs missed some PRs for some reason that I currently don't understand. Commits which could be found with a simple git log -- src/qt were missing when running the above command.

Later, when all the PRs that I catched and backported are merged into Dash, I will go through all Bitcoin PRs/commits again omitting the filtering from the above commands (with some custom scripting instead). I'll create additional PRs with these missing PRs/commits then. This may also include many non-Qt/Gui related PRs.

The next step will be to run a full comparison between Dash and Bitcoin and look for all kinds of suspicions differences between Dash and Bitcoin 0.14. This should reveal the last missing PRs to backport.

My long-term target is to get as close as possible to the Bitcoin code again, only omitting stuff that is not desired in Dash (like Segwit code).

EDIT: forgot to add list of included PRs. Working on it now.

laanwj and others added 10 commits September 5, 2017 17:23
a3c3ddb [Qt] add InMempool() info to transaction details (Jonas Schnelli)
fa5769e [qt] Fix misleading translation (MarcoFalke)
fa8c8d7 torcontrol debug: Change to a blanket message that covers both cases (MarcoFalke)
6fd0a07 Remove hardcoded fee from CoinControl ToolTip (fanquake)
5fdf32d Replace some instances of formatWithUnit with formatHtmlWithUnit (fanquake)
a5a0831 Double semicolon cleanup. (21E14)
fa989fb [qt] coincontrol workaround is still needed in qt5.4 (fixed in qt5.5) (MarcoFalke)
9d263bd Typo fixes in comments (Chris Wheeler)
… the console window

43abb02 [Qt] Add a new chevron/arrow icon for the console prompt line (Jonas Schnelli)
56c9e66 [Qt] keep scroll position in GUI console after changing font size (Jonas Schnelli)
3a3a927 [Qt] Add option to increase/decrease font size in the console window (Jonas Schnelli)
b51ed40 QT: Add 'copy full transaction details' option (Eric Shaw)
21e45a0 Fix history deletion bug after font change (Andrew C)
@codablock
Copy link
Author

I've just seen that Github was smart enough to reference all original PRs in the commit list. Is this enough for review or should I still add a list to the description?

@codablock codablock changed the title [WIP] Backport Bitcoin Qt/Gui changes up to 0.14.x part 1 Backport Bitcoin Qt/Gui changes up to 0.14.x part 1 Sep 5, 2017
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

I've just seen that Github was smart enough to reference all original PRs in the commit list. Is this enough for review or should I still add a list to the description?

I don't have any preferences, having list of PRs in github's commits list is ok imo.

Re PR itself: Tested, works as expected 👍
I have one issue I'd like to be fixed though, see inline comment

QDateTime date = QDateTime::fromTime_t(static_cast<uint>(rec->time));
QString txLabel = walletModel->getAddressTableModel()->labelForAddress(QString::fromStdString(rec->address));

details.append(date.toString("M/d/yy HH:mm"));
Copy link

Choose a reason for hiding this comment

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

IMO, it's confusing that date format here doesn't match the one on tx tab. I would propose to make sure they match, i.e.:

diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
index 28d67933b..497e374f6 100644
--- a/src/qt/transactiontablemodel.cpp
+++ b/src/qt/transactiontablemodel.cpp
@@ -637,10 +637,9 @@ QVariant TransactionTableModel::data(const QModelIndex &index, int role) const
     case TxPlainTextRole:
         {
             QString details;
-            QDateTime date = QDateTime::fromTime_t(static_cast<uint>(rec->time));
             QString txLabel = walletModel->getAddressTableModel()->labelForAddress(QString::fromStdString(rec->address));
 
-            details.append(date.toString("M/d/yy HH:mm"));
+            details.append(formatTxDate(rec));
             details.append(" ");
             details.append(formatTxStatus(rec));
             details.append(". ");

Copy link
Author

Choose a reason for hiding this comment

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

Agree. Just checked formatTxDate and it uses the system locale, as it should be. Added a commit on top.

@UdjinM6 UdjinM6 added this to the 12.2 milestone Sep 6, 2017
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

ACK

@codablock
Copy link
Author

Btw, even though I might have missed some PRs, all PRs required up until the progress overlay are done in my local branch. As far as I could see, everything works as expected. After this PR, there are 51 more commits which I'd split up in multiple PRs.

You wrote in Slack that you plan to do feature freeze this week. Would you consider waiting for these things to be merged in as well?

@UdjinM6
Copy link

UdjinM6 commented Sep 6, 2017

Would you consider waiting for these things to be merged in as well?

Can't promise anything. If it's more or less trivial we can merge them I guess. We can always revert them back before the release if anything is wrong (and merge fixed one in later versions e.g. 0.12.2.1).

@codablock
Copy link
Author

Well trivial if you look at the amount of changed lines...not really :D
But most changes are luckily local to the Qt/Gui part and well tested on Bitcoin as I'd assume. Also, the changes didn't interfere much with Dash specific code. The only thing that interfered much with Dash code was the fee handling in the Gui, that is something I left out for now as it would need some discussion.

@UdjinM6 UdjinM6 merged commit 690cb58 into dashpay:v0.12.2.x Sep 7, 2017
@codablock codablock deleted the backport_bitcoin_qt_1 branch September 14, 2018 12:49
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.

4 participants