Added icon policy documentation#178
Closed
Bosch-0 wants to merge 1639 commits intobitcoin-core:masterfrom
Closed
Conversation
jarolrod
reviewed
Jan 9, 2021
Contributor
jarolrod
left a comment
There was a problem hiding this comment.
Concept ACK 8131d80
PR was moved because the original branch was deleted: Link to previous conversation
The current commit fails the lint-whitespace.sh linter with error message:
This diff appears to have added new lines with trailing whitespace.
Per the ci failure, It appears that the culprit may be on line 59.
maflcko
reviewed
Jan 10, 2021
|
|
||
| - Open source tool [ImageOptim](https://imageoptim.com/api). | ||
| - Open source tool [Trimage](https://trimage.org/). | ||
| - (Advanced) Run them through Bitcoin Core's [optimize-png.py](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/master/optimise-pngs.py) script. |
Contributor
There was a problem hiding this comment.
I'd say it is a requirement for any image to be added to this repo to be cleaned and compressed deterministically via this script.
Author
There was a problem hiding this comment.
Is there a way this script could be run on images automatically as they are added to the repo? The result between the 3 tools should be the same (they all use zopflippng / pngcrush). The only reason I added ImageOptim and Trimage is so designers can do this task if they wish - some may not have the technical skills to use the python script.
Contributor
There was a problem hiding this comment.
@Bosch-0 it seems like the best thing to do here is to include a small guide on processing images through the included script before inclusion into the repo
luke-jr
suggested changes
Jan 13, 2021
Member
luke-jr
left a comment
There was a problem hiding this comment.
Probably should add a section on required licensing
| #### SVG Source File | ||
| SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support. | ||
| In the event that different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool such as: | ||
| Inkspace, Adobe Illustrator, Figma, Sketch, or Adobe XD. |
Member
There was a problem hiding this comment.
Let's remove non-free software from this list. It would be annoying if we had SVGs that only render correctly in them.
Author
There was a problem hiding this comment.
From experience SVGs exported from all of this tools are the same and interoperable. Happy to remove the adobe products though, most designers use Figma (which is free) these days anyways.
Contributor
There was a problem hiding this comment.
I don't think we HAVE to remove the non-free tools, but maybe mention that Figma is the preferred tool?
Member
There was a problem hiding this comment.
Figma is non-free and definitely not preferred...
Actually, the preferred tool (rsvg-convert) isn't even listed right now!
Member
There was a problem hiding this comment.
I would suggest dropping "such as: Inkspace, Adobe Illustrator, Figma, Sketch, or Adobe XD" to avoid update maintenance and review effort over time on this.
Author
There was a problem hiding this comment.
Figma is free, the paid version is only for a few extra features that do not pertain to SVG rendering. It is the preferred tool among designers (who I was pointing to in this doc as they are usually the ones who hand on the file to developers in a production sense).
@jonatack I agree, tools come and go - it's easy enough to google and use any vector tool you wish, will make this change.
For some reference here is some info around UX/UI tools usage - https://uxtools.co/survey-2020/.
Member
There was a problem hiding this comment.
"Free" deals with source code and licensing terms, NOT money (you're thinking "free of charge").
| - An icon used in production is being replaced with a new icon. | ||
| - A feature is being added to the Bitcoin Core GUI that requires the addition of a new icon. | ||
|
|
||
| If a new icon is designed for an upcoming or active PR, the designer of the icon should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) to get feedback on the design and to make sure it is consistent with Bitcoin Core's iconography. |
Member
There was a problem hiding this comment.
I'm not sure this needs an extra issue independent from the PR using it?
Author
There was a problem hiding this comment.
Posting an issue would be for requesting an icon be desgined to fit the PR (if the dev who did the PR is not also a designer). Though I guess this could be coordinated over IRC or other channels.
Contributor
There was a problem hiding this comment.
I think it's useful to indicate under what conditions icons can be added to Bitcoin Core
Member
|
Concept ACK. Not going to ACK due to the lack of my expertise in this particular field. |
hebasto
reviewed
Feb 22, 2021
|
|
||
|
|
||
|
|
||
| ## Attribution |
Member
There was a problem hiding this comment.
https://cirrus-ci.com/task/5544742043779072?command=lint#L949
Suggested change
| ## Attribution | |
| ## Attribution |
jarolrod
reviewed
Mar 4, 2021
Contributor
jarolrod
left a comment
There was a problem hiding this comment.
Some further suggestions
| each icon. | ||
|
|
||
| #### SVG Source File | ||
| SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support. |
Contributor
There was a problem hiding this comment.
the comma isn't actually needed here
Suggested change
| SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support. | |
| SVGs are used as source files because they can scale while retaining image quality but are not used in production due to limited application support. | |
|
|
||
| #### SVG Source File | ||
| SVGs are used as source files because they can scale while retaining image quality, but are not used in production due to limited application support. | ||
| In the event that different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool such as: |
Contributor
There was a problem hiding this comment.
nit, the sentence can be simplified a bit here
Suggested change
| In the event that different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool such as: | |
| If different-sized production (PNG) icons are required, they can be generated from the associated SVG source file in a vector-based tool such as: | |
| Inkspace, Adobe Illustrator, Figma, Sketch, or Adobe XD. | ||
|
|
||
| #### PNG Production File | ||
| PNGs are used in production due to wide application support, transparency support, and better image quality in comparison to competing file types |
Contributor
There was a problem hiding this comment.
nit, s/in comparison/compared
Suggested change
| PNGs are used in production due to wide application support, transparency support, and better image quality in comparison to competing file types | |
| PNGs are used in production due to wide application support, transparency support, and better image quality compared to competing file types | |
|
|
||
|
|
||
| ### Icon Grid | ||
| Bitcoin Core uses an 8-point grid system for its icons which utilizes size increments of 8px to visually orientate elements within a grid. The majority of screen sizes are divisible by 8 which makes fitting elements easier using an 8-point grid system. This makes things more efficient for designers and maintains consistency across applications. |
Contributor
There was a problem hiding this comment.
needs a comma, adding 'system' makes the antecedent clearer
Suggested change
| Bitcoin Core uses an 8-point grid system for its icons which utilizes size increments of 8px to visually orientate elements within a grid. The majority of screen sizes are divisible by 8 which makes fitting elements easier using an 8-point grid system. This makes things more efficient for designers and maintains consistency across applications. | |
| Bitcoin Core uses an 8-point grid system for its icons which utilizes size increments of 8px to visually orientate elements within a grid. The majority of screen sizes are divisible by 8, which makes fitting elements easier using an 8-point grid system. This system makes things more efficient for designers and maintains consistency across applications. | |
| Icon style should adhere to the following principles to maintain consistency and clarity: | ||
|
|
||
| - To ensure icons look good at any scale, a minimalistic design should be used. | ||
| - Icon size should be 32x32px by default. A different sized icon may be necessary under special circumstances. In the event that the 32x32px size is too small for the required icon placement, then the icon should be scaled up or down while adhering to the 8-point grid system. |
Contributor
There was a problem hiding this comment.
nit
Suggested change
| - Icon size should be 32x32px by default. A different sized icon may be necessary under special circumstances. In the event that the 32x32px size is too small for the required icon placement, then the icon should be scaled up or down while adhering to the 8-point grid system. | |
| - Icon size should be 32x32px by default. A different sized icon may be necessary under special circumstances. If the 32x32px size is too small for the required icon placement, then the icon should be scaled up or down while adhering to the 8-point grid system. | |
|
|
||
| - To ensure icons look good at any scale, a minimalistic design should be used. | ||
| - Icon size should be 32x32px by default. A different sized icon may be necessary under special circumstances. In the event that the 32x32px size is too small for the required icon placement, then the icon should be scaled up or down while adhering to the 8-point grid system. | ||
| - Icon should avoid organic shapes or elements, these do not scale well. |
Contributor
There was a problem hiding this comment.
Suggested change
| - Icon should avoid organic shapes or elements, these do not scale well. | |
| - The icon should avoid organic shapes or elements; these do not scale well. | |
|
|
||
|
|
||
| ### Optimizing Production Files (PNG): | ||
| Production (PNG) files must be optimized before their inclusion in Bitcoin Core. PNG optimization removes various unnecessary color profiles, ancillary (alla), and text chunks resulting in a lossless reduction of the file's size. Any `zopflipng` / `pngcrush` based PNG optimizer can be used. Below are some examples: |
Contributor
There was a problem hiding this comment.
Suggested change
| Production (PNG) files must be optimized before their inclusion in Bitcoin Core. PNG optimization removes various unnecessary color profiles, ancillary (alla), and text chunks resulting in a lossless reduction of the file's size. Any `zopflipng` / `pngcrush` based PNG optimizer can be used. Below are some examples: | |
| Production (PNG) files must be optimized before their inclusion in Bitcoin Core. PNG optimization removes various unnecessary color profiles, ancillary (alla), and text chunks, resulting in a lossless reduction of the file's size. Any `zopflipng` / `pngcrush` based PNG optimizer can be used. Below are some examples: | |
| - An icon used in production is being replaced with a new icon. | ||
| - A feature is being added to the Bitcoin Core GUI that requires the addition of a new icon. | ||
|
|
||
| If a new icon is designed for an upcoming or active PR, the designer of the icon should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) to get feedback on the design and to make sure it is consistent with Bitcoin Core's iconography. |
Contributor
There was a problem hiding this comment.
Suggested change
| If a new icon is designed for an upcoming or active PR, the designer of the icon should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) to get feedback on the design and to make sure it is consistent with Bitcoin Core's iconography. | |
| If a new icon is designed for an upcoming or active PR, the icon designer should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) to get feedback on the design and make sure it is consistent with Bitcoin Core's iconography. | |
|
|
||
| If a new icon is designed for an upcoming or active PR, the designer of the icon should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) to get feedback on the design and to make sure it is consistent with Bitcoin Core's iconography. | ||
|
|
||
| If a new feature which requires a new icon is being added to the Bitcoin Core GUI, the developer should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) requesting the new icon be designed. |
Contributor
There was a problem hiding this comment.
Suggested change
| If a new feature which requires a new icon is being added to the Bitcoin Core GUI, the developer should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) requesting the new icon be designed. | |
| If a new feature is being added which requires a new icon, the developer should [open an issue](https://github.com/bitcoin-core/gui/issues/new/choose) requesting the new icon be designed. | |
|
|
||
| Icons, both SVG and PNG, are not to be added to Bitcoin Core prior to a production use case. | ||
|
|
||
| When opening a PR that intends to add an icon, a SVG source file must be included under `src/qt/res/src`, and a optimized production file (PNG) must be included under `src/qt/res/icons`. Both source and production files should be 32x32px in size. |
Contributor
There was a problem hiding this comment.
Suggested change
| When opening a PR that intends to add an icon, a SVG source file must be included under `src/qt/res/src`, and a optimized production file (PNG) must be included under `src/qt/res/icons`. Both source and production files should be 32x32px in size. | |
| When opening a PR that intends to add an icon, an SVG source file must be included under `src/qt/res/src`, and an optimized production file (PNG) must be included under `src/qt/res/icons`. Both source and production files should be 32x32px in size. | |
faaf395 fuzz: Extend psbt fuzz target a bit (MarcoFalke) Pull request description: Previously it only merged the psbt with itself, now it tries to merge another. ACKs for top commit: practicalswift: Tested ACK faaf395 Tree-SHA512: e1b1d31a47d35e1767285bc2fda176c79cb0550d6d383fe467104272e61e1c83f6cbc0c7d6bbc0c3027729eec13ae1f289f8950117ee91e0fb3703e66d5e6918
Windows and macOS do not support the global mouse selection.
fa41a91 ci: Run self-hosted ci (MarcoFalke) fa52a40 ci: Make cirrus cache folders relative to cirrus base dir (MarcoFalke) fa27841 ci: Restart docker before run (MarcoFalke) fad4f48 ci: [refactor] Create setting for ephemeral config in .cirrus.yml (MarcoFalke) Pull request description: Due to our heavy use of the Cirrus CI community cluster, some tasks may take a long time to get scheduled. While it is possible to use "Compute Credits" to get immediately scheduled on the cluster, I couldn't find a sponsor that'd be willing to cover the total cost, if all tasks were paid for with credits. However, it is also possible to bring our own runners to Cirrus CI. For testing purposes, a single task will be transformed to run on the DrahtBot infrastructure. If all goes well, the other tasks can be moved, too. ACKs for top commit: hebasto: ACK fa41a91, I have reviewed the code and it looks OK. Tree-SHA512: 513738daec36da8cd48a8f11e687ff0b7dfaba1ae4ed2fa77e7b043f88fd52bf5c0dbad2768e13df88518323917f08348cb62be6376a423142921f8d2c59a938
d3b0b08 doc: release notes for new listbanned fields (Jarol Rodriguez) 60290d3 test: increase listbanned unit test coverage (Jon Atack) 3e978d1 rpc: add time_remaining field to listbanned (Jarol Rodriguez) 5456b34 rpc: add ban_duration field to listbanned (Jarol Rodriguez) c95c616 doc: improve listbanned help (Jarol Rodriguez) dd3c8ea rpc: swap position of banned_until and ban_created fields (Jarol Rodriguez) Pull request description: This PR adds a `ban_duration` and `time_remaining` field to the `listbanned` RPC command. Thanks to jonatack, this PR also expands the `listbanned` test coverage to include these new fields It's useful to keep track of `ban_duration` as this is another data point on which to sort banned peers. I found this helpful in adding additional context columns to the GUI `bantablemodel` as part of a follow-up PR. As [suggested](bitcoin/bitcoin#21602 (comment)) by jonatack, `time_remaining` is another useful user-centric data point. Since a ban always expires after its created, the `ban_created` field is now placed before the `banned_until` field. This new ordering is more logical. This PR also improves the `help listbanned` output by providing additional context to the descriptions of the `address`, `ban_created`, and `banned_until` fields. **Master: listbanned** ``` [ { "address": "1.2.3.4/32", "banned_until": 1617691101, "ban_created": 1617604701 }, { "address": "135.181.41.129/32", "banned_until": 1649140716, "ban_created": 1617604716 } ] ``` **PR: listbanned** ``` [ { "address": "1.2.3.4/32", "ban_created": 1617775773, "banned_until": 1617862173, "ban_duration": 86400, "time_remaining": 86392 }, { "address": "3.114.211.172/32", "ban_created": 1617753165, "banned_until": 1618357965, "ban_duration": 604800, "time_remaining": 582184 } ] ``` ACKs for top commit: jonatack: re-ACK d3b0b08 hebasto: ACK d3b0b08, tested on Linux Mint 20.1 (x86_64). MarcoFalke: review ACK d3b0b08 🕙 Tree-SHA512: 5b83ed2483344e546d57e43adc8a1ed7a1fff292124b14c86ca3a1aa2aec8b0f7198212fabff2c5145e7f726ca04ae567fe667b141254c7519df290cf63774e5
The order was broken in commit ffff4e7
Since Qt 5.3.1 hash seeding is disabled for rcc. See commit 5283a6c87beac5a43f612786fefd6e43f2c70bf6.
Fix make deploy for arm64-darwin
The intent here is to allow checking ComputeBlockVersion behaviour with each deployment, rather than only testdummy on mainnet. This commit does the trivial refactoring component of that change.
This generalises the ComputeBlockVersion test so that it can apply to any activation parameters we might set, and checks all the parameters set for each deployment on each chain, to simultaneously ensure that the deployments we have configured work sensibly, and that the test code does not suffer bitrot in the event that all interesting deployments are buried.
Simplify the versionbits unit test slightly to make the next set of changes a little easier to follow.
Previously we used deployments that would timeout prior to Bitcoin's invention, which allowed the deployment to still be activated in unit tests. This switches those deployments to be truly never active.
This removes the DEFINED->FAILED transition and changes the STARTED->FAILED transition to only occur if signalling didn't pass the threshold. This ensures that it is always possible for activation to occur, no matter what settings are chosen, or the speed at which blocks are found.
6ba8921 refactor + document coin selection strategy (glozow) 58ea324 [docs] add doxygen comments to wallet code (glozow) 0c74716 [docs] format existing comments as doxygen (glozow) Pull request description: I think it would help code review to have more documentation + doxygen comments ACKs for top commit: Xekyo: ReACK bitcoin/bitcoin@6ba8921 achow101: ACK 6ba8921 Tree-SHA512: 74a78d9b0e0c1d5659bed566432a5b3511511d8b2432f440565f443da7b8257a1b90e70aa7505a7f8abf618748eeb43d166e84f278bdee3d34ce5d5c37dc573a
…it.py fad6269 test: Assert that exit code indicates failure (MarcoFalke) faecb72 test: Fix intermittent issue in p2p_segwit.py (MarcoFalke) Pull request description: Calling `start_node` might call `wait_for_rpc_connection`, which will fail. https://cirrus-ci.com/task/5669555591708672?logs=ci#L3504 ``` File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_segwit.py", line 1974, in test_upgrade_after_activation self.start_node(2, extra_args=["-reindex", f"-segwitheight={SEGWIT_HEIGHT}"]) File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 508, in start_node node.wait_for_rpc_connection() File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection raise FailedToStartError(self._node_msg( test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status 1 during initialization ACKs for top commit: jnewbery: ACK fad6269 dhruv: ACK fad6269 Tree-SHA512: 4c5e39ce25e135717ea433258518f93f09d1c528c4538a8627d3da13bc0c0ba4b45911703c26392ff0f5e0cb7831a6c7cc53e6e29102d3da9c8cfce7cef333cc
…domized base address in MSVC builds 9bd3f35 build: adds switch for disabling random base addresses in MSVC (Ethan Heilman) Pull request description: In m4 builds we have the --disable-hardening switch that can be given in `./configure` to turn off randomized addresses. This PR provides a simple way of turning off randomized addresses in MSVC builds. This PR: * Adds this option the common-init project file so that it can be globally set across the project * Documents this switch in msvc build readme I have run the following test to verify this works I ran the msvc build with `<RandomizedBaseAddress>true</RandomizedBaseAddress>` then checked `bitcoind.exe` and `bitcoin-cli.exe` with `dumpbin.exe`: bitcoind ``` > .\dumpbin.exe /headers src/bitcoind.exe Microsoft (R) COFF/PE Dumper Version 14.16.27045.0 ... OPTIONAL HEADER VALUES 20B magic # (PE32+) 14.28 linker version AE4600 size of code 345C00 size of initialized data 0 size of uninitialized data 6BED74 entry point (00000001406BED74) mainCRTStartup 1000 base of code 140000000 image base (0000000140000000 to 0000000140E2DFFF) 1000 section alignment 200 file alignment 6.00 operating system version 0.00 image version 6.00 subsystem version 0 Win32 version E2E000 size of image 400 size of headers 0 checksum 3 subsystem (Windows CUI) 8160 DLL characteristics High Entropy Virtual Addresses Dynamic base NX compatible Terminal Server Aware ``` bitcoin-cli ``` > .\dumpbin.exe /headers src/bitcoin-cli.exe Microsoft (R) COFF/PE Dumper Version 14.16.27045.0 ... OPTIONAL HEADER VALUES 20B magic # (PE32+) 14.28 linker version 1E3E00 size of code 92C00 size of initialized data 0 size of uninitialized data 104384 entry point (0000000140104384) mainCRTStartup 1000 base of code 140000000 image base (0000000140000000 to 0000000140279FFF) 1000 section alignment 200 file alignment 6.00 operating system version 0.00 image version 6.00 subsystem version 0 Win32 version 27A000 size of image 400 size of headers 0 checksum 3 subsystem (Windows CUI) 8160 DLL characteristics High Entropy Virtual Addresses Dynamic base NX compatible Terminal Server Aware ``` Then I built with `<RandomizedBaseAddress>false</RandomizedBaseAddress>` then checked `bitcoind.exe` and `bitcoin-cli.exe` with `dumpbin.exe` and observed that `Dynamic base` was longer listed in `OPTIONAL HEADER VALUES` bitcoind ``` PS C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.16.27023\bin\HostX64\x64> .\dumpbin.exe /headers C:\Users\e0\Documents\GitHub\bitcoin021noaslr/src/bitcoind.exe Microsoft (R) COFF/PE Dumper Version 14.16.27045.0 ... OPTIONAL HEADER VALUES 20B magic # (PE32+) 14.28 linker version AE4600 size of code 33FE00 size of initialized data 0 size of uninitialized data 6BED74 entry point (00000001406BED74) mainCRTStartup 1000 base of code 140000000 image base (0000000140000000 to 0000000140E27FFF) 1000 section alignment 200 file alignment 6.00 operating system version 0.00 image version 6.00 subsystem version 0 Win32 version E28000 size of image 400 size of headers 0 checksum 3 subsystem (Windows CUI) 8120 DLL characteristics High Entropy Virtual Addresses NX compatible Terminal Server Aware ``` bitcoin-cli ``` > .\dumpbin.exe /headers src/bitcoin-cli.exe Microsoft (R) COFF/PE Dumper Version 14.16.27045.0 ... OPTIONAL HEADER VALUES 20B magic # (PE32+) 14.28 linker version 1E3E00 size of code 90C00 size of initialized data 0 size of uninitialized data 104384 entry point (0000000140104384) mainCRTStartup 1000 base of code 140000000 image base (0000000140000000 to 0000000140277FFF) 1000 section alignment 200 file alignment 6.00 operating system version 0.00 image version 6.00 subsystem version 0 Win32 version 278000 size of image 400 size of headers 0 checksum 3 subsystem (Windows CUI) 8120 DLL characteristics High Entropy Virtual Addresses NX compatible Terminal Server Aware ``` ACKs for top commit: sipsorcery: ACK 9bd3f35. practicalswift: cr ACK 9bd3f35: patch looks correct Tree-SHA512: ddffdb4ff8a09c7cfef61c07a5db2a2828e9e3aa795ad8e5a1bf51ab489a68b40f87f6694518c5e0b8858c0fad4f93bb947b052e6b9d5e55eb38e764b746fc02
…e in intro dialog 415fb2e GUI/Intro: Move prune setting below explanation (Luke Dashjr) 2a84c6b GUI/Intro: Estimate max age of backups that can be restored with pruning (Luke Dashjr) e2dcd95 GUI/Intro: Rework UI flow to let the user set prune size in GBs (Luke Dashjr) f2e5a6b GUI/Intro: Abstract GUI-to-option into Intro::getPrune (Luke Dashjr) 62932cc GUI/Intro: Return actual prune setting from showIfNeeded (Luke Dashjr) Pull request description:  Moved from bitcoin/bitcoin#18728 ACKs for top commit: ryanofsky: Code review ACK 415fb2e. Changes since last review: mb/gib suffixes, constexpr QOverload expected_backup_days tweaks, new moveonly layout commit jarolrod: Tested ACK 415fb2e. Talkless: tACK 415fb2e, tested on Debian Sid with Qt 5.15.2. hebasto: ACK 415fb2e, my unresolved comments are not blockers, and they could be resolved in follow ups. Tree-SHA512: bd4882a9c08e6a6eb14b7fb6366983db8581425b4949fea212785d34d8fad9e32fb81ca8c8cdbfb2c05ea394aaf5a746ba2cf16623795c7252c3bdb61d455f00
61fd8fe Remove progress bar on modal overlay (bruno) Pull request description: This PR removes the progress bar (keeping only the percentage) on modal overlay resolves bitcoin-core#279 Before:  After:  ACKs for top commit: Bosch-0: tACK bitcoin-core@61fd8fe on Windows 10. Unnecessary Progress bar no longer there :) jarolrod: tACK 61fd8fe Tree-SHA512: 96d72f168b26e950ce37e9f489bcbcc608473c44bce3be127ccd47d17b7642fa234d314596186ee16b430d943575c312d84133425507a17ae7ac58ecae986639
5f96d7d rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr) 23fe504 test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr) 90c966b rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr) b936239 index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr) bb7788b test: Test coinstatsindex robustness across restarts (Fabian Jahr) e0938c2 test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr) 2501576 rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr) 655d929 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack) ca01bb8 rpc: Add Coinstats index to getindexinfo (Fabian Jahr) 57a026c test: Add unit test for Coinstats index (Fabian Jahr) 6a4c0c0 test: Add functional test for Coinstats index (Fabian Jahr) 3f166ec rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr) 3c914d5 index: Coinstats index can be activated with command line flag (Fabian Jahr) dd58a4d index: Add Coinstats index (Fabian Jahr) a8a46c4 refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr) 9c8a265 refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr) 2e2648a crypto: Make MuHash Remove method efficient (Fabian Jahr) Pull request description: This is part of the coinstats index project tracked in #18000 While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step. Features summary: - Users can start their node with the option `-coinstatsindex` which syncs the index in the background - After the index is synced the user can use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index - The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height ACKs for top commit: Sjors: re-tACK 5f96d7d jonatack: Code review re-ACK 5f96d7d per `git range-diff 13d27b4 07201d3 5f96d7d` promag: Tested ACK 5f96d7d. Light code review ACK 5f96d7d. Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
Contributor
|
@Bosch-0 are you going to revisit this? would be nice to have this in if we're going to attempt a migration to https://github.com/BitcoinDesign/Bitcoin-Icons |
e94920a qt: peertableview alternating row colors (randymcmillan) Pull request description: peers-tab: enable alternating row colors for peer table and banned table ACKs for top commit: Bosch-0: tACK bitcoin-core@e94920a on Windows 10 - works as intended. Before / after below: jarolrod: tACK e94920a Tree-SHA512: 05ba18e1db9700bbd68644fe02292409f4e5c52e301b1b2977c335d1ff16456a93fb0b15c8c8385d1b15f648141341990706d530f6b08ecb33098fa941b9af1f
Contributor
|
Concept ACK (dependent on comments being addressed) |
…on macOS cf971c9 build: use -isysroot over --sysroot on macOS (fanquake) Pull request description: Not only does this seem to be the more correct behaviour when targeting Darwin, but if you use `-isysroot`, Clangs Darwin driver will [infer the deployment target](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L1652) from the SDK and use other SDK info when parsing arguments to the linker. In the case of [`-platform_version`](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L2656), which is added if the linker is [new enough](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L342), the version tuple is constructed from the SDKInfo, and SDKInfo, as far as I can tell, only exists when `-isysroot` has been passed, see [parseSDKSettings](https://github.com/llvm/llvm-project/blob/3e037f8f0e26acab8cc784ea4c7d05da79f7c22e/clang/lib/Driver/ToolChains/Darwin.cpp#L1785) As a result, the SDK version field in the `LC_BUILD_VERSION` command is filled out. i.e when building master: ```bash cmd LC_BUILD_VERSION cmdsize 32 platform 1 minos 10.14 sdk n/a ntools 1 tool 3 version 650.9 ``` vs this PR: ```bash cmd LC_BUILD_VERSION cmdsize 32 platform 1 minos 10.14 sdk 11.3 ntools 1 tool 3 version 650.9 ``` This, from what I understand, will fix the issue we are having with Qt deciding wether or not to enable features like "Dark mode" on macOS, see #21771, however I have not tested that. Thus this is an alternative to #21782. Our usage of `--sysroot` was added in #17118. ```bash -isysroot <dir> Set the system root directory (usually /) ``` ACKs for top commit: Sjors: tACK cf971c9 hebasto: re-ACK cf971c9, only rebased and addressed comments since my [previous](bitcoin/bitcoin#21793 (review)) review. Tree-SHA512: f01138179fb85083b5505bbaa48810451098ffa4da5d3c9b673785448790aa76f2e64b2aab6e698f6ee378a21f70626445a3fabee7c61dbfc44e96f3e3964656
…-getinfo with -rpcwallet=unloaded wallet returns no balances
…li.py c5bb142 test: resolve bug in test/functional/interface_bitcoin_cli.py - Test -getinfo with -rpcwallet=unloaded wallet returns no balances (klementtan) Pull request description: I think there is a bug in this test case where the new value of `cli_get_info` is not asserted. ACKs for top commit: jonatack: ACK c5bb142 Tree-SHA512: 50c0c2c8fe63c95f951dee892fbacedf92208f47efe5ed481fbb255f15137c799d9200fa3ff31a442df0691248d7ff04d899842722c3032cd7f35553622ba38c
This PR was motivated by a comment in GUI PR (257) regarding a suggested improvement not being supported by VS2017. When checking whether master can still be built with the VS2017 toolset ABI issues were encountered. Most likely due to the pre-compiled Qt binaries that are used. It does not seem worth the effort to try and support VS2017, which would most likely require additional Qt binaries, or lengthy instructions on how to build static Qt binaries on Windows (which is very error prone and tedious). Added advisory note about build not working with earlier Visual Studio versions. Fixed grammar.
… from readme 0a33145 Remove Visual Studio 2017 reference from readme (Aaron Clauson) Pull request description: This PR was motivated by a comment in GUI PR (257) regarding a suggested improvement not being supported by VS2017. When checking whether master can still be built with the VS2017 toolset ABI issues were encountered. Most likely due to the pre-compiled Qt binaries that are used. It does not seem worth the effort to try and support VS2017, which would most likely require additional Qt binaries, or lengthy instructions on how to build static Qt binaries on Windows (which is very error prone and tedious). ACKs for top commit: jarolrod: ACK 0a33145 hebasto: ACK 0a33145, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 17e2326cd55a5cc3fc13622ba72bb44f9b9d55070cb77941df5fd943cd3f1baf02b9ac9504dfae5941c522748ad7b584c3c8da03fb323a87b3639eb926ce1699
…bitcoin.conf and files.md 54133c5 doc: add indexes/coinstats/db/ to files.md (Jon Atack) 5d1050f doc: fix -coinstatsindex help, and test/rpc touchups (Jon Atack) e041ee0 doc: add coinstatsindex to bitcoin.conf (Jon Atack) Pull request description: ACKs for top commit: Sjors: utACK 54133c5 MarcoFalke: cr ACK 54133c5 clarkmoody: utACK 54133c5 Tree-SHA512: 1a7f3e89873b7dc79ec71d5d39e9e3e4977ce43cc4bee208ad55291bef1bb319a9d1c34ed84a87d6a803db983bdfd0af4d9f396cec0bec86b1701ebbb6f34378
Author
|
Incorrectly rebased, @jarolrod will re-pick this up |
hebasto
added a commit
to bitcoin-core/gui-qml
that referenced
this pull request
Nov 2, 2021
7335bdd doc: introduce icon policy documentation (bosch) Pull request description: Moved from bitcoin-core/gui#310 Picking up bitcoin-core/gui#178 This introduces documented policies for how icons should be prepared, optimized, styled, contributed, and attributed within Bitcoin Core. Having guidelines for iconography helps with making applications more consistent and efficient. [document render](https://github.com/bitcoin-core/gui-qml/blob/23ad199b43e9068d0571eba5965752b03842a7f4/src/qml/doc/icon-policy.md) ACKs for top commit: hebasto: re-ACK 7335bdd Tree-SHA512: aebdfcfea83d23584ccbcf7bdb706bcddfd63dbdb53018ec8220cab87cc92720fd41cb532325e258fff6bcc53b5cebbe498fba2ad95ebebb5da4f868f92607d3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
19 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I created some documentation as to how icons should be prepared, optimized, styled, contributed and attributed within Bitcoin Core. Having guidelines for iconography helps with making applications more consistent and efficient.
The current icon set does not yet follow this policy but I will be updating them in future PRs. To see the current icon set using this policy see this figma source file..
Made a mistake with the last PR open for this #143 and had to re-open.