windows: show progress bar in task bar

alexeyneu commented at 10:26 am on August 28, 2018: none

progress bar over task bar thumbnail button
Windows only
untitled 1

laanwj commented at 10:42 am on August 28, 2018: member

120 bucks (address removed)

thank you for your first time contribution!

however, it is not-done to solicit donations on github, so I’ve had to remove the address

DrahtBot commented at 10:56 am on August 28, 2018: member

#11625 (Add BitcoinApplication & RPCConsole tests by ryanofsky)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

alexeyneu force-pushed on Aug 28, 2018

in src/qt/modaloverlay.cpp:37 in db2a5d30aa outdated

35     setVisible(false);
36 }
37 
38 ModalOverlay::~ModalOverlay()
39-{
40+{

scravy commented at 12:11 pm on August 28, 2018:

you added a fair bit of trailing white space here ;-)

scravy commented at 12:33 pm on August 28, 2018:

this is actually failing your travis build in the lint stage – This diff appears to have added new lines with trailing whitespace. – https://travis-ci.org/bitcoin/bitcoin/jobs/421514716#L719

alexeyneu commented at 1:37 pm on August 28, 2018:

should work if it were only unwanted thing

alexeyneu commented at 12:18 pm on August 28, 2018: none

linux #ifdef ’s were missing. it’s reloaded some time ago. @laanwj ok ,now you can copy-paste my wallet address by double click , line near location , github profile. I bet you wanna to

alexeyneu force-pushed on Aug 28, 2018

in src/qt/winshutdownmonitor.cpp:72 in 9cc9bd700e outdated

82+
83+		default:
84+				if (pMsg->message == WM_ret && com_b++ > 1 )
85+				{						
86+					hwnd = pMsg->hwnd;
87+					if (com_b == 3)  com_u = std::thread(&hammer ,(LPVOID)NULL);

MarcoFalke commented at 2:14 pm on August 28, 2018:

Don’t forget to install clang-format and run the https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy script to preempt whitespace nitpicking.

alexeyneu commented at 5:40 pm on August 29, 2018:

looks like travis is stuck. there’s \nLF there.

alexeyneu force-pushed on Aug 28, 2018

in src/qt/winshutdownmonitor.cpp:62 in 891838056a outdated

70-           }
71-       }
72+				StartShutdown();
73+				*pnResult = FALSE;
74+				return true;
75+				break;

donaloconnor commented at 7:56 pm on August 28, 2018:

Why are the break statements put in after the return?

alexeyneu commented at 9:15 pm on August 28, 2018:

fine to read . Just checked petzold , yeah return end is the common practice

scravy commented at 6:29 pm on August 30, 2018:

@alexeyneu this is not resolved. The break after a return is dead code and should be removed.

alexeyneu commented at 7:59 pm on August 30, 2018:

@scravy Did you look to the left side of diff?

alexeyneu commented at 8:19 pm on August 30, 2018:

and you’d stay silent all this time? how it comes

alexeyneu commented at 3:58 pm on August 31, 2018:

It’s here for a reason .You need either left one or mine . Otherwise build here starts to fail.

scravy commented at 4:10 pm on August 31, 2018:

Yes, what about it?

scravy commented at 4:11 pm on August 31, 2018:

long story short: There is no actual change here. Just don’t touch these lines and it’s less code to discuss, reason, build-error about.

alexeyneu commented at 5:14 pm on August 31, 2018:

so if i marked it as resolved then it is

in src/qt/winshutdownmonitor.cpp:69 in 891838056a outdated

79+				break;
80+
81+#ifdef ENABLE_WALLET
82+
83+		default:
84+				if (pMsg->message == WM_ret && com_b++ > 1 )

donaloconnor commented at 8:00 pm on August 28, 2018:

What’s the purpose of com_b here? This code is very confusing

alexeyneu commented at 10:05 pm on August 28, 2018:

to show that first two windows are not used. i’m not sure ++com_b > 2 will be a lot better. short-circuit evaluation itself is not a problem i guess. atomic_int is just for fun there .

in src/qt/winshutdownmonitor.cpp:107 in 891838056a outdated

102+UINT CALLBACK hammer(VOID *c)
103+{
104+	if(com_r.use_count() &&  *com_r.lock() > 184) return 0;
105+	int reserve = 0;
106+	CoInitializeEx(NULL, COINIT_MULTITHREADED); //used only by extra thread for now
107+	CoCreateInstance(CLSID_TaskbarList ,NULL ,CLSCTX_INPROC_SERVER ,IID_ITaskbarList3 ,(LPVOID*)&bhr); //pointer grabs smth finally

donaloconnor commented at 8:01 pm on August 28, 2018:

IID_ITaskbarList3 is only available on Windows 7+. I think Bitcoin Core supports Win Vista+. This will probably fail to initialise and return a null bhr. We should ensure this succeeds.

alexeyneu commented at 9:26 pm on August 28, 2018:

i’m not sure about vista also. fo’ sho’ fail , but did you here promise to support it

in src/qt/winshutdownmonitor.cpp:100 in 891838056a outdated

 95@@ -67,4 +96,27 @@ void WinShutdownMonitor::registerShutdownBlockReason(const QString& strReason, c
 96     else
 97         qWarning() << "registerShutdownBlockReason: Failed to register: " + strReason;
 98 }
 99+
100+#ifdef ENABLE_WALLET
101+
102+UINT CALLBACK hammer(VOID *c)
103+{
104+	if(com_r.use_count() &&  *com_r.lock() > 184) return 0;

donaloconnor commented at 8:03 pm on August 28, 2018:

184 magic number?

alexeyneu commented at 9:28 pm on August 28, 2018:

2.5% left . Or you’ll have green shade all the time.

scravy commented at 6:30 pm on August 30, 2018:

To make this apparently magic number more self describing, maybe extract it into a constant that has an explanatory name or add a comment describing why 184.

alexeyneu commented at 6:53 pm on August 30, 2018:

for now i wait successful build. Does it worth it?

in src/qt/modaloverlay.cpp:130 in 891838056a outdated

123@@ -122,6 +124,9 @@ void ModalOverlay::tipUpdate(int count, const QDateTime& blockDate, double nVeri
124     // show the percentage done according to nVerificationProgress
125     ui->percentageProgress->setText(QString::number(nVerificationProgress*100, 'f', 2)+"%");
126     ui->progressBar->setValue(nVerificationProgress*100);
127+ #if defined(Q_OS_WIN)
128+   *sp_tray = nVerificationProgress*190;

donaloconnor commented at 8:08 pm on August 28, 2018:

What’s the 190?

alexeyneu commented at 9:31 pm on August 28, 2018:

size of button on 4k i bet . SetProgressValue’s last arg

alexeyneu commented at 10:37 pm on August 28, 2018:

about QWinTaskbarProgress something tells me you have reasons not to use it haha

scravy commented at 8:50 am on August 29, 2018:

the button as in the application icon in the task bar? In what unit, pixels? And if so, will that width change if you for instance put your task bar on the left or on the right (where they look completely different and are as horizontally spacious as you want).

The only problem I can see with QWinTaskbarProgress is that is seems to be there only since Qt 5.2 and it’s part of winextras.

donaloconnor commented at 8:57 am on August 29, 2018:

Also if button sizes are taken into consideration, then DPI scaling should also be considered

alexeyneu commented at 9:17 am on August 29, 2018:

it’s just scale division , like chrono::milliseconds You could put 1190 with np . If you put say 30 …. I bet you’d understand already

alexeyneu commented at 9:30 am on August 29, 2018:

yeah that’s about hsize in pixels. @scravy about latter , trust me it’s not the only problem it will fail

donaloconnor changes_requested

donaloconnor commented at 8:08 pm on August 28, 2018: contributor

Concept ACK but… I find this code very difficult to follow in its current form. There are a lot of magic numbers, variable names that are confusing etc.

Also should we consider using QWinTaskbarProgress ?

alexeyneu force-pushed on Aug 29, 2018

laanwj renamed this:
~~progress bar~~
[windows] progress bar in task bar
on Aug 29, 2018

alexeyneu force-pushed on Aug 29, 2018

alexeyneu force-pushed on Aug 30, 2018

in src/qt/bitcoin.cpp:10 in 90cdcbef51 outdated

 4@@ -5,9 +5,7 @@
 5 #if defined(HAVE_CONFIG_H)
 6 #include <config/bitcoin-config.h>
 7 #endif
 8-
 9 #include <qt/bitcoingui.h>
10-

luke-jr commented at 1:12 pm on August 30, 2018:

Please don’t remove unrelated whitespace.

alexeyneu commented at 1:26 pm on August 30, 2018:

that’s not funny

luke-jr commented at 1:35 pm on August 30, 2018:

It wasn’t meant to be funny…?

promag commented at 6:19 pm on August 30, 2018:

@alexeyneu you should only commit what is relevant to the PR.

Edit: please update the code according to developer notes.

alexeyneu commented at 6:36 pm on August 30, 2018:

who you mean by developer?And what about this pair of your tools , does its build results matter? And one of you two for sure should know do you provide vista support?

scravy commented at 6:45 pm on August 30, 2018:

@alexeyneu https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

scravy commented at 6:47 pm on August 30, 2018:

This document gives a clue about the expected compatibility: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes.md#compatibility

alexeyneu commented at 6:54 pm on August 30, 2018:

so no vista

in src/qt/winshutdownmonitor.cpp:66 in 90cdcbef51 outdated

74+				return true;
75+				break;
76+		case WM_ENDSESSION:
77+				*pnResult = FALSE;
78+				return true;
79+				break;

scravy commented at 6:29 pm on August 30, 2018:

same here

in src/qt/modaloverlay.h:43 in 90cdcbef51 outdated

52 
53 protected:
54-    bool eventFilter(QObject * obj, QEvent * ev);
55-    bool event(QEvent* ev);
56+	bool eventFilter(QObject * obj, QEvent * ev);
57+	bool event(QEvent* ev);

scravy commented at 6:30 pm on August 30, 2018:

it is unnecessary to replace the space indentation with tabs indentation, please don’t.

promag commented at 6:57 pm on August 30, 2018: member

Concept ACK and agree with @donaloconnor. The implementation should be decoupled as much as possible.

Also should we consider using QWinTaskbarProgress ?

That requires another dependency QT += winextras (?) and Qt 5.2.

promag commented at 8:09 pm on August 30, 2018: member

After seeing QWinTaskbarProgress I think we should use it instead of adding more OS specific code — that’s what Qt is for?

jonasschnelli commented at 8:15 pm on August 30, 2018: contributor

Agree with @promag, using QWinTaskbarProgress makes far more sense to me.

luke-jr commented at 8:33 pm on August 30, 2018: member

@alexeyneu What is your native language?

Can you do this using QWinTaskbarProgress?

alexeyneu commented at 9:41 pm on August 30, 2018: none

Russian .
No i wont help you on that. Unrelated to that: my opinion - it’s a bad idea

promag commented at 10:23 pm on August 30, 2018: member

@alexeyneu you mean it’s a bad idea to use QWinTaskbarProgress?

alexeyneu commented at 10:21 am on August 31, 2018: none

fo’ sho’ yoo’ll not have this bar outside windows os . You’ll mire down with this stuff you offer.

alexeyneu force-pushed on Aug 31, 2018

laanwj added the label Windows on Aug 31, 2018

alexeyneu force-pushed on Sep 1, 2018

progress bar bbd5aa3e36

alexeyneu force-pushed on Sep 2, 2018

alexeyneu commented at 1:06 am on September 2, 2018: none

it’s ready

fanquake renamed this:
~~[windows] progress bar in task bar~~
windows: show progress bar in task bar
on Sep 2, 2018

fanquake commented at 8:46 am on September 2, 2018: member

I’ve updated the PR title and removed unrelated text from the description. @alexeyneu This is not ready for merge yet. Please update your commit with a more meaningful commit message, and at least fix any linting issues, see here.

However I also agree with the commenters above that using QWinTaskbarProgress would seem like a better idea than the current implementation. So I don’t think this will be merged in it’s current state. Can you also justify not using it with more information than:

Unrelated to that: my opinion - it’s a bad idea

alexeyneu commented at 10:35 am on September 2, 2018: none

@fanquake check chat before writing this bot-like stuff
https://botbot.me/freenode/bitcoin-core-dev/2018-09-01/?msg=104023171&page=2

scravy commented at 10:40 am on September 2, 2018: contributor

@alexeyneu I don’t know what your point is in linking this chatlog. Are you referring to the build which has no errors but the other stages passed?

If you compare that build with your build you will notice that in your build the checks on different platforms have not even been executed. This is because the lint stage failed and it failed correctly. You are messing up white space for no reason and this has been pointed out several times in this pull request already, you are just choosing to neglect these comments.

alexeyneu commented at 1:01 pm on September 2, 2018: none

@scavy dont show up here any more

fanquake commented at 1:44 pm on September 2, 2018: member

@alexeyneu Given that you’ve chosen to ignore review comments and constructive criticism, asked for donations multiple times, called people “morons” and told others not to “show up here any more”, I’m inclined to close this pull request.

I appreciate there is a language barrier, and this is your first time contributing (which can be tough), so if you want to keep working on this PR, and discuss, in detail, the downsides to using QWinTaskbarProgress, lets do that.

However, keep the comments constructive and on topic, otherwise this PR will be closed.

sipa commented at 2:30 pm on September 2, 2018: member

@alexeyneu Just to be clear: regarding your earlier comment on IRC, the Bitcoin Core project does not have funds for paying for contributions. If your patch has a price, please close it.

There are multiple ways of being funded to contribute to open source projects, but posting a BTC address and putting a price on a PR is not one of them. You could find someone to sponsor your time, or otherwise employ you to contribute.

Also, you’re testing people’s patience for all the reasons listed by @fanquake above. We’re happy for your contributions, but if you want to see your patch accepted, please don’t dismiss review comments from maintainers and others, and be respectful.

alexeyneu commented at 4:20 pm on September 2, 2018: none

@fanquake This code has a price . Dont lie about asking or digits 120 may appear on your forehead someday.

promag commented at 4:41 pm on September 2, 2018: member

Thanks, but no thanks. NACK current implementation and IMO not worth it to have such a specific OS feature unless abstracted by Qt.

sipa commented at 4:58 pm on September 2, 2018: member

In that case, we have no interest in your code.

donaloconnor commented at 5:02 pm on September 2, 2018: contributor

NACK.

I helped review and follow the discussions in good faith. I believed it was a language barrior. Plenty of others would be more than happy to contribute something like this and be nice and unconditional about it.

sipa closed this on Sep 2, 2018

fanquake locked this on Sep 2, 2018

windows: show progress bar in task bar #14090