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

gui: Add Windows taskbar progress #14137

Closed
wants to merge 1 commit into from
Closed

gui: Add Windows taskbar progress #14137

wants to merge 1 commit into from

Conversation

ken2812221
Copy link
Contributor

ref: #14090
Close #14134

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

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

Should this also show splashscreen progress?

}
QWinTaskbarProgress* taskbar_progress = m_taskbar_button->progress();
if (nVerificationProgress < 1.0) {
taskbar_progress->setValue(100.0 * nVerificationProgress + 0.5);
Copy link
Member

Choose a reason for hiding this comment

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

Why + 0.5? (add a comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is as same as here , I guess this is to round the value

progressBar->setValue(nVerificationProgress * 1000000000.0 + 0.5);

Copy link
Member

Choose a reason for hiding this comment

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

Hm, okay.

Copy link
Member

Choose a reason for hiding this comment

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

Use qRound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@fanquake
Copy link
Member

fanquake commented Sep 3, 2018

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.

@jonasschnelli
Copy link
Contributor

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

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

Much better, thanks!

@@ -19,6 +19,9 @@
#include <QMenu>
#include <QPoint>
#include <QSystemTrayIcon>
#ifdef Q_OS_WIN
#include <QWinTaskbarButton>
Copy link
Member

Choose a reason for hiding this comment

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

Use follow declaration instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean "follow declaration"?

Copy link
Member

Choose a reason for hiding this comment

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

Ops, forward declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
QWinTaskbarProgress* taskbar_progress = m_taskbar_button->progress();
if (nVerificationProgress < 1.0) {
taskbar_progress->setValue(100.0 * nVerificationProgress + 0.5);
Copy link
Member

Choose a reason for hiding this comment

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

Use qRound?

@ken2812221
Copy link
Contributor Author

ken2812221 commented Sep 4, 2018

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

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

@ken2812221
Copy link
Contributor Author

ken2812221 commented Sep 4, 2018

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

@maflcko
Copy link
Member

maflcko commented Sep 4, 2018

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

@bitcoin bitcoin deleted a comment from DrahtBot Sep 5, 2018
@maflcko
Copy link
Member

maflcko commented Sep 5, 2018

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

@ken2812221
Copy link
Contributor Author

ken2812221 commented Sep 6, 2018

I may know what's going on. makefile would check only the source file listed in .stamp file

bitcoin/depends/Makefile

Lines 153 to 158 in adf27b5

define check_or_remove_sources
mkdir -p $($(package)_source_dir); cd $($(package)_source_dir); \
test -f $($(package)_fetched) && ( $(build_SHA256SUM) -c $($(package)_fetched) >/dev/null 2>/dev/null || \
( echo "Checksum missing or mismatched for $(package) source. Forcing re-download."; \
rm -f $($(package)_all_sources) $($(1)_fetched))) || true
endef

@ken2812221
Copy link
Contributor Author

ken2812221 commented Sep 6, 2018

@MarcoFalke This should work now.

@donaloconnor
Copy link
Contributor

tACK c3c0022 on Win10

image

@jonasschnelli
Copy link
Contributor

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

bildschirmfoto 2018-09-25 um 20 42 55

@ken2812221
Copy link
Contributor Author

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

@jonasschnelli
Copy link
Contributor

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

@ken2812221
Copy link
Contributor Author

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.

@bitcoin bitcoin deleted a comment from DrahtBot Sep 25, 2018
@maflcko
Copy link
Member

maflcko commented Sep 25, 2018

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

@ken2812221 ken2812221 changed the title gui: Add Windows taskbar progress [WIP] gui: Add Windows taskbar progress Oct 6, 2018
@ken2812221
Copy link
Contributor Author

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

@ken2812221 ken2812221 changed the title [WIP] gui: Add Windows taskbar progress gui: Add Windows taskbar progress Oct 7, 2018
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 20, 2018

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.

@@ -159,7 +159,8 @@ endef

define check_or_remove_sources
mkdir -p $($(package)_source_dir); cd $($(package)_source_dir); \
test -f $($(package)_fetched) && ( $(build_SHA256SUM) -c $($(package)_fetched) >/dev/null 2>/dev/null || \
test -f $($(package)_fetched) && ( test `cat $($(package)_fetched) | wc -l` -eq $(words $($(package)_all_sources)) && \
$(build_SHA256SUM) -c $($(package)_fetched) >/dev/null 2>/dev/null || \
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Needs rebase

tar --strip-components=1 -xf $($(package)_source_dir)/$($(package)_qttools_file_name) -C qttools
tar --strip-components=1 -xf $($(package)_source_dir)/$($(package)_qttools_file_name) -C qttools && \
mkdir qtwinextras && \
tar --strip-components=1 -xf $($(package)_source_dir)/$($(package)_qtwinextras_file_name) -C qtwinextras
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -16,8 +16,12 @@ $(package)_qttranslations_sha256_hash=9822084f8e2d2939ba39f4af4c0c2320e45d599676
$(package)_qttools_file_name=qttools-$($(package)_suffix)
$(package)_qttools_sha256_hash=50e75417ec0c74bb8b1989d1d8e981ee83690dce7dfc0c2169f7c00f397e5117

$(package)_qtwinextras_file_name=qtwinextras-$($(package)_suffix)
$(package)_qtwinextras_sha256_hash=794090446d33eecf9eb3f3a4f6f13b3e7ab5307936f8d21e4a76e7b6ba79125d
Copy link
Member

Choose a reason for hiding this comment

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

This hash needs to be updated for the current version.

@fanquake
Copy link
Member

fanquake commented Jul 3, 2019

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 fanquake closed this Jul 3, 2019
@NicolasDorier
Copy link
Contributor

I will probably pick it up once we get #15529

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

windows: Show sync progress in task bar
10 participants