windows: show progress bar in task bar#14090
windows: show progress bar in task bar#14090alexeyneu wants to merge 1 commit intobitcoin:masterfrom
Conversation
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 |
Note to reviewers: This pull request conflicts with the following ones:
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. |
src/qt/modaloverlay.cpp
Outdated
There was a problem hiding this comment.
you added a fair bit of trailing white space here ;-)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
should work if it were only unwanted thing
|
linux |
src/qt/winshutdownmonitor.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
looks like travis is stuck. there's \nLF there.
donaloconnor
left a comment
donaloconnor
left a comment
There was a problem hiding this comment.
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 ?
src/qt/winshutdownmonitor.cpp
Outdated
There was a problem hiding this comment.
What's the purpose of com_b here? This code is very confusing
There was a problem hiding this comment.
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 .
src/qt/winshutdownmonitor.cpp
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i'm not sure about vista also. fo' sho' fail , but did you here promise to support it
|
Concept ACK and agree with @donaloconnor. The implementation should be decoupled as much as possible.
That requires another dependency |
|
After seeing QWinTaskbarProgress I think we should use it instead of adding more OS specific code — that's what Qt is for? |
|
Agree with @promag, using |
|
@alexeyneu What is your native language? Can you do this using |
|
Russian . |
|
@alexeyneu you mean it's a bad idea to use |
|
fo' sho' yoo'll not have this bar outside windows os . You'll mire down with this stuff you offer. |
|
it's ready |
fanquake
changed the title
|
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
|
|
@fanquake check chat before writing this bot-like stuff |
|
@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. |
|
@Scavy dont show up here any more |
|
@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 However, keep the comments constructive and on topic, otherwise this PR will be closed. |
|
@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. |
|
@fanquake This code has a price . Dont lie about asking or digits 120 may appear on your forehead someday. |
|
Thanks, but no thanks. NACK current implementation and IMO not worth it to have such a specific OS feature unless abstracted by Qt. |
|
In that case, we have no interest in your code. |
|
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. |
11 participants
progress bar over task bar thumbnail button
Windows only