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

qt: Remove hidden columns in coin control dialog #14828

Merged
merged 1 commit into from Dec 4, 2018

Conversation

Projects
None yet
6 participants
@promag
Copy link
Member

commented Nov 28, 2018

Instead of having hidden columns, store the data in specific roles.

Overlaps with #14817, fixes #11811.

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

Ah @laanwj already suggested this approach in #11811 (comment)

@fanquake fanquake added the GUI label Nov 28, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 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:

  • #14817 (qt: Remove unnecessary columns in Coin Selection window (#11811) by hebasto)

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.

@hebasto

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

Concept ACK.

Show resolved Hide resolved src/qt/coincontroldialog.cpp Outdated

@promag promag force-pushed the promag:2018-11-coincontroldialog branch Nov 28, 2018

@hebasto

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

There are some concerns about 4cd9a3a772807df825c27c410410c3ea96b59371 commit.

QTreeView::uniformRowHeights:

This property should only be set to true if it is guaranteed that all items in the view has the same height. This enables the view to do some optimizations.

  1. IMO, it is not a good idea to mix a bugfix commit with an optimization commit in one PR (see Pull Request Philosophy).
  2. There is no obvious need for such optimization.
  3. How can 'the same height' condition be guaranteed now and then?
  4. How can I test this optimization and measure the effect of it?

Could this optimization commit be moved out of this PR's scope?

@promag promag force-pushed the promag:2018-11-coincontroldialog branch Nov 28, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

@hebasto removed commit.

Show resolved Hide resolved src/qt/coincontroldialog.cpp Outdated
@hebasto

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

tACK 06cc0412da8fc3d031e804e7df0ae5b9686663b0 after nit

@promag promag force-pushed the promag:2018-11-coincontroldialog branch to 1c28feb Nov 29, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

Ah @laanwj already suggested this approach in #11811 (comment)

Yes, using column metadata is definitely the more idiomatic approach, thanks for doing this.

utACK 1c28feb

@hebasto

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

re- tACK 1c28feb

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Tested ACK 1c28feb

@jonasschnelli jonasschnelli merged commit 1c28feb into bitcoin:master Dec 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Dec 4, 2018

Merge #14828: qt: Remove hidden columns in coin control dialog
1c28feb qt: Remove hidden columns in coin control dialog (João Barbosa)

Pull request description:

  Instead of having hidden columns, store the data in specific roles.

  Overlaps with #14817, fixes #11811.

Tree-SHA512: e86e9ca426b9146ac28997ca1920dbae6cc4e2e494ff94fe131d605cd6c013183fc5de10036c886a4d6dcae497ac4067de3791be0ef9c88f7ce9f57f7bd97422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.