Skip to content

Remove reindex special case from the progress bar label - #697

Merged
maflcko merged 1 commit into
bitcoin-core:masterfrom
maflcko:2301-gui-reindex-👖
Feb 7, 2023

Hidden character warning

The head ref may contain hidden characters: "2301-gui-reindex-\ud83d\udc56"
Merged

Remove reindex special case from the progress bar label#697
maflcko merged 1 commit into
bitcoin-core:masterfrom
maflcko:2301-gui-reindex-👖

Conversation

@maflcko

Copy link
Copy Markdown
Contributor

The user knows which option they passed to the program, so it seems overly verbose to offer the user feedback whether or not they passed -reindex. Treat it as DISK, like all other cases that are treated as DISK:

  • -reindex-chainstate
  • -loadblock

@DrahtBot

DrahtBot commented Jan 17, 2023

Copy link
Copy Markdown
Contributor

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

Reviews

See the guideline for information on the review process.

TypeReviewers
ACKjohn-moffett, hebasto
Stale ACKLarryRuane

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

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.

@maflcko

Copy link
Copy Markdown
ContributorAuthor

For reference:

  • -reindex-chainstate remains unchanged:
    rcs

  • -loadblock remains unchanged:
    lblk

  • -reindexstage 2 remains unchanged:
    reindex_same_2

  • -reindexstage 1 (master):
    reindex_master

  • -reindexstage 1 (this pull):
    reindex_1_pull

@maflckomaflcko added the UI All about "look and feel" label Jan 18, 2023
@maflcko

Copy link
Copy Markdown
ContributorAuthor

So the only user-visible change should be changing the word "Reindex" to "Index"

@hebasto

Copy link
Copy Markdown
Member

Concept ACK.

As this PR touches code outside the src/qt directory, i.e., src/interfaces/node.h and src/node/interfaces.cpp, should it be moved to the main repo for a broader reviewing?

@maflcko

Copy link
Copy Markdown
ContributorAuthor

It is only changing an interface that the gui uses, so I think it should be fine

@hebasto

Copy link
Copy Markdown
Member

cc @ryanofsky

@LarryRuaneLarryRuane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

code-review, tested ACK fa20044

I ran src/qt/bitcoin-qt -noconnect -signet -reindex with and without the PR, and the status message (lower-left corner) for stage 1 was as expected. I didn't try reindex-chainstate, loadblock, or reindex stage 2.

Comment on lines +151 to +152
if (m_node.isLoadingBlocks()) return BlockSource::DISK;
if (getNumConnections() > 0) return BlockSource::NETWORK;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this, somehow else following return (or break, continue) doesn't seem right.

@john-moffettjohn-moffett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK fa20044

Nit: Maybe remove the one remaining unnecessary enum use for BlockSource?

enum BlockSource blockSource = clientModel->getBlockSource();

@maflcko

Copy link
Copy Markdown
ContributorAuthor

Thanks, removed and rebased. Should be trivial to re-ACK with git range-diff

@john-moffett

Copy link
Copy Markdown
Contributor

Re-ACK faff2ba

@hebastohebasto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK faff2ba, I have reviewed the code and it looks OK, I agree it can be merged.

@maflcko
maflcko merged commit 1bcabe6 into bitcoin-core:masterFeb 7, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 7, 2023
@LarryRuane

Copy link
Copy Markdown
Contributor

post-merge re-ACK

@maflcko
maflcko deleted the 2301-gui-reindex-👖 branch February 8, 2023 10:57
@bitcoin-corebitcoin-core locked and limited conversation to collaborators Feb 8, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

UIAll about "look and feel"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko@DrahtBot@hebasto@john-moffett@LarryRuane