gui: Add Windows taskbar progress

ken2812221 commented at 5:21 am on September 3, 2018: contributor

This PR adds an progress bar at taskbar on Windows. This progress bar will hide when full synced.

fanquake added the label GUI on Sep 3, 2018

fanquake added the label Windows on Sep 3, 2018

in src/qt/bitcoingui.cpp:903 in ab7336a161 outdated

898+    if (m_taskbar_button->window() == nullptr) {
899+        m_taskbar_button->setWindow(windowHandle());
900+    }
901+    QWinTaskbarProgress* taskbar_progress = m_taskbar_button->progress();
902+    if (nVerificationProgress < 1.0) {
903+        taskbar_progress->setValue(100.0 * nVerificationProgress + 0.5);

luke-jr commented at 6:07 am on September 3, 2018:

Why + 0.5? (add a comment)

ken2812221 commented at 6:39 am on September 3, 2018:

This is as same as here , I guess this is to round the value https://github.com/bitcoin/bitcoin/blob/68f3c7eb080e461cfeac37f8db7034fe507241d0/src/qt/bitcoingui.cpp#L856

luke-jr commented at 6:49 am on September 3, 2018:

Hm, okay.

promag commented at 12:45 pm on September 3, 2018:

Use qRound?

ken2812221 commented at 1:24 pm on September 3, 2018:

Ok, I’ll use qRound after gitian build done.

luke-jr commented at 6:09 am on September 3, 2018: member

Should this also show splashscreen progress?

fanquake commented at 7:52 am on September 3, 2018: member

Concept ACK. Thanks for following up on this so quickly, and using QWinTaskbarProgress .

Given that this is an entirely new implementation, and includes the build system changes, I don’t think you need to cherry-pick/include the other committer.

I’ll test on a Windows VM shortly.

fanquake added the label Needs gitian build on Sep 3, 2018

jonasschnelli commented at 11:10 am on September 3, 2018: contributor

Concept ACK (haven’t looked at the code though)

in src/qt/bitcoingui.h:23 in ab7336a161 outdated

18@@ -19,6 +19,9 @@
19 #include <QMenu>
20 #include <QPoint>
21 #include <QSystemTrayIcon>
22+#ifdef Q_OS_WIN
23+#include <QWinTaskbarButton>

promag commented at 12:44 pm on September 3, 2018:

Use follow declaration instead?

ken2812221 commented at 1:09 pm on September 3, 2018:

What do you mean “follow declaration”?

promag commented at 1:28 pm on September 3, 2018:

Ops, forward declaration.

ken2812221 commented at 2:46 pm on September 3, 2018:

Why?

donaloconnor commented at 2:58 pm on September 3, 2018:

Because you don’t need to pull in the entire header file (+ what ever it includes) when using just a pointer. It’s preferred in case like this to reduce compile times etc. Include it in CPP instead.

promag commented at 3:16 pm on September 3, 2018:

☝️also, look in other headers, it’s already a recurring practice.

ken2812221 commented at 4:38 pm on September 3, 2018:

Oh, I saw that QT_NAMESPACE. I’ll do that after gitian build finished too.

promag commented at 12:47 pm on September 3, 2018: member

Much better, thanks!

DrahtBot removed the label Needs gitian build on Sep 3, 2018

ken2812221 commented at 1:00 am on September 4, 2018: contributor

I have no idea what’s going on in gitian build. I am able to build this locally. Why does qtwinextras not exist?

0sha256sum: /home/ubuntu/cache/common/qtwinextras-opensource-src-5.9.6.tar.xz: No such file or directory
1/home/ubuntu/cache/common/qtwinextras-opensource-src-5.9.6.tar.xz: FAILED open or read
2sha256sum: WARNING: 1 listed file could not be read

ken2812221 force-pushed on Sep 4, 2018

ken2812221 commented at 2:17 pm on September 4, 2018: contributor

a99f9caf92346d9c0bf823b2e3c2bcc44f7d9c6a6205050b985e63f5e3ed0ee1 bitcoin-0.17.99-win32-debug.zip 044347527dd20eb4081cdbc9acd8adc05a97925854169affcf191b915abeef8d bitcoin-0.17.99-win32-setup-unsigned.exe 61b745860cd74c0fb93c8cc1fbfae897589b88084f3d298688ff4ec16e4782f1 bitcoin-0.17.99-win32.zip fbbc87b50c16cd027aeb9a1ccb5a16320c414f92252c4312b605223cb4673e7a bitcoin-0.17.99-win64-debug.zip 9b2b7536863379e4977d8dcf80486ee2343e60a7fda5a20c39aaff03ba66b5c2 bitcoin-0.17.99-win64-setup-unsigned.exe 50ab47bbb18eae55fc994b3d5b274208cbeae4e379b75092e34501b4aa35d485 bitcoin-0.17.99-win64.zip 92eaedf473b4bc36dc3524d57d2ea5cee1afa12999458b0ce352e422dbe26f61 bitcoin-win-0.18-build.assert

This is my gitian build result for 954d19aff18dc128e3208275b65facd7b55cd852 using ./gitian-build.py -bnDpj4 -m6000 -ow 0 14137

MarcoFalke added the label Needs gitian build on Sep 4, 2018

MarcoFalke commented at 4:44 pm on September 4, 2018: member

I may have to purge the cache? Did a rm -r gitian-builder/cache/

DrahtBot removed the label Needs gitian build on Sep 5, 2018

MarcoFalke deleted a comment on Sep 5, 2018

MarcoFalke commented at 1:34 pm on September 5, 2018: member

@jonasschnelli Could you try to see if your gitian build script also fails here?

ken2812221 commented at 9:47 am on September 6, 2018: contributor

I may know what’s going on. makefile would check only the source file listed in .stamp file https://github.com/bitcoin/bitcoin/blob/adf27b531a7fcd7066ab6649e8073bd1895a823a/depends/Makefile#L153-L158

ken2812221 force-pushed on Sep 6, 2018

ken2812221 commented at 11:50 am on September 6, 2018: contributor

@MarcoFalke This should work now.

MarcoFalke deleted a comment on Sep 6, 2018

MarcoFalke added the label Needs gitian build on Sep 6, 2018

DrahtBot removed the label Needs gitian build on Sep 7, 2018

donaloconnor commented at 9:58 pm on September 8, 2018: contributor

tACK https://github.com/bitcoin/bitcoin/pull/14137/commits/c3c00220a86e5bbbfe7a494d323388edbf9c7803 on Win10

jonasschnelli commented at 6:43 pm on September 25, 2018: contributor

Tested on Win10 via gitian build https://bitcoin.jonasschnelli.ch/build/797 but can’t see the status bar:

ken2812221 commented at 6:47 pm on September 25, 2018: contributor

The status bar would hide on 100%. Should I make it always be visible?

jonasschnelli commented at 7:05 pm on September 25, 2018: contributor

@ken2812221: Okay. I see. Any good test plan how to test this on regtest?

ken2812221 commented at 7:12 pm on September 25, 2018: contributor

Okay. I see. Any good test plan how to test this on regtest?

Hmm. I don’t think there is a good way to make it not to be 100% on regtest. I would have to use testnet and -reindex to test it.

MarcoFalke deleted a comment on Sep 25, 2018

MarcoFalke commented at 7:13 pm on September 25, 2018: member

I guess you’d have to set nTime of the blocks to last year or so?

ken2812221 force-pushed on Oct 6, 2018

ken2812221 renamed this:
~~gui: Add Windows taskbar progress~~
[WIP] gui: Add Windows taskbar progress
on Oct 6, 2018

ken2812221 force-pushed on Oct 6, 2018

ken2812221 commented at 3:37 pm on October 6, 2018: contributor

Update: Now the taskbar progress show/hide along with the modaloverlay. It can be easily tested on regtest.

ken2812221 renamed this:
~~[WIP] gui: Add Windows taskbar progress~~
gui: Add Windows taskbar progress
on Oct 7, 2018

DrahtBot commented at 10:30 am on October 20, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15581 (depends: Make less assumptions about build env by dongcarl)
#15277 ([Help Wanted] contrib: Enable building in Guix containers by dongcarl)
#15112 (build: Optionally enable -Wzero-as-null-pointer-constant by Empact)
#14920 (Build: enable -Wdocumentation via isystem by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in depends/Makefile:163 in 699a2fffd3 outdated

158@@ -159,7 +159,8 @@ endef
159 
160 define check_or_remove_sources
161   mkdir -p $($(package)_source_dir); cd $($(package)_source_dir); \
162-  test -f $($(package)_fetched) && ( $(build_SHA256SUM) -c $($(package)_fetched) >/dev/null 2>/dev/null || \
163+  test -f $($(package)_fetched) && ( test `cat $($(package)_fetched) | wc -l` -eq $(words $($(package)_all_sources)) && \
164+    $(build_SHA256SUM) -c $($(package)_fetched) >/dev/null 2>/dev/null || \

fanquake commented at 10:44 am on October 20, 2018:

Why is this change required?

ken2812221 commented at 10:54 am on October 20, 2018:

See above. The current build system only detect the first file is changed or not, so winextra would not be downloaded in you have the source file cache, but it would be re-built(which cause an error). This make the Makefile detect the file amount to be exactly match.

DrahtBot added the label Needs rebase on Nov 10, 2018

gui: Add Windows taskbar progress 18eb4dbb8a

ken2812221 force-pushed on Nov 10, 2018

DrahtBot removed the label Needs rebase on Nov 10, 2018

DrahtBot commented at 4:00 pm on March 18, 2019: member

DrahtBot added the label Needs rebase on Mar 18, 2019

in depends/packages/qt.mk:128 in 18eb4dbb8a

125   tar --strip-components=1 -xf $($(package)_source_dir)/$($(package)_qttranslations_file_name) -C qttranslations && \
126   mkdir qttools && \
127-  tar --strip-components=1 -xf $($(package)_source_dir)/$($(package)_qttools_file_name) -C qttools
128+  tar --strip-components=1 -xf $($(package)_source_dir)/$($(package)_qttools_file_name) -C qttools && \
129+  mkdir qtwinextras && \
130+  tar --strip-components=1 -xf $($(package)_source_dir)/$($(package)_qtwinextras_file_name) -C qtwinextras

MarcoFalke commented at 4:46 pm on March 18, 2019:

Needs rebase to add the --no-same-owner

in depends/packages/qt.mk:20 in 18eb4dbb8a

15@@ -16,8 +16,12 @@ $(package)_qttranslations_sha256_hash=9822084f8e2d2939ba39f4af4c0c2320e45d599676
16 $(package)_qttools_file_name=qttools-$($(package)_suffix)
17 $(package)_qttools_sha256_hash=50e75417ec0c74bb8b1989d1d8e981ee83690dce7dfc0c2169f7c00f397e5117
18 
19+$(package)_qtwinextras_file_name=qtwinextras-$($(package)_suffix)
20+$(package)_qtwinextras_sha256_hash=794090446d33eecf9eb3f3a4f6f13b3e7ab5307936f8d21e4a76e7b6ba79125d

luke-jr commented at 7:13 pm on April 22, 2019:

This hash needs to be updated for the current version.

fanquake added the label Up for grabs on Jul 3, 2019

fanquake commented at 6:14 am on July 3, 2019: member

Closing as up for grabs. @sipsorcery, @NicolasDorier either of you might be interested in picking this up if you think it will be an improvement to Qt on Windows.

fanquake closed this on Jul 3, 2019

NicolasDorier commented at 12:14 pm on July 3, 2019: contributor

I will probably pick it up once we get https://github.com/bitcoin/bitcoin/pull/15529

fanquake removed the label Up for grabs on Oct 21, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

gui: Add Windows taskbar progress #14137

Conflicts