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

[GUI] Load persisted transaction filter during start #1287

Merged
merged 1 commit into from
Jul 22, 2020

Conversation

Mrs-X
Copy link

@Mrs-X Mrs-X commented Jan 17, 2020

The transaction filter settings were persisted, but never loaded when the wallet starts.

Since it would be tricky to compute the index of an item of the filter-combobox from the filter value this change does it in the other direction, it persists the index of the item and computes the filter value from it which is trivial.

@Mrs-X Mrs-X added this to the 4.1.0 milestone Jan 17, 2020
@Mrs-X Mrs-X self-assigned this Jan 17, 2020
Fuzzbawls
Fuzzbawls previously approved these changes Jan 22, 2020
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 87cf0c2

@Fuzzbawls
Copy link
Collaborator

Additional comment: maybe it would be good to store/load the ordering selection to compliment this PR.

@Mrs-X
Copy link
Author

Mrs-X commented Jan 28, 2020

Addressed the @random-zebra nit, and implemented the @Fuzzbawls proposal to also persist the ordering selection.

@Mrs-X Mrs-X force-pushed the Persist_TRX_Filter_Settings branch from b22f7c4 to 33780bf Compare February 24, 2020 21:39
@Mrs-X
Copy link
Author

Mrs-X commented Feb 24, 2020

@random-zebra It's indeed redundant.
Removed and squashed...

@furszy furszy modified the milestones: 4.1.0, Future Apr 10, 2020
@random-zebra random-zebra modified the milestones: Future, 5.0.0 Apr 24, 2020
@Fuzzbawls Fuzzbawls force-pushed the Persist_TRX_Filter_Settings branch from 33780bf to aecd93e Compare July 2, 2020 02:49
@Fuzzbawls
Copy link
Collaborator

Rebased and resolved the conflicts

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Aside from a couple non-blocking nits, I'm currently having the comboBoxSortType set to an empty label when first starting up the wallet with this PR.

Screenshot from 2020-07-09 17-31-32

src/qt/pivx/dashboardwidget.cpp Outdated Show resolved Hide resolved
src/qt/pivx/dashboardwidget.cpp Outdated Show resolved Hide resolved
@Fuzzbawls Fuzzbawls force-pushed the Persist_TRX_Filter_Settings branch 2 times, most recently from 535bc00 to 89041b8 Compare July 10, 2020 00:32
@Fuzzbawls
Copy link
Collaborator

I'm currently having the comboBoxSortType set to an empty label when first starting up the wallet with this PR.

Screenshot from 2020-07-09 17-31-32

This is due to a change in what is being stored in the transactionType QSettings object. I've added a simple check that will reset this setting to 0 ("ALL") if it's current value is out of range.

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Tested, works as advertised.
Minor suggestion about the check for filterIndex value out of range.

@Fuzzbawls Fuzzbawls force-pushed the Persist_TRX_Filter_Settings branch from 89041b8 to 7169d54 Compare July 11, 2020 04:38
random-zebra
random-zebra previously approved these changes Jul 11, 2020
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK 7169d54

@random-zebra random-zebra requested a review from furszy July 11, 2020 09:37
@random-zebra random-zebra added the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Jul 11, 2020
@@ -301,7 +338,7 @@ void DashboardWidget::onSortTypeChanged(const QString& value)

// Store settings
QSettings settings;
settings.setValue("transactionType", filterByType);
settings.setValue("transactionType", filterIndex);
Copy link

Choose a reason for hiding this comment

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

We should store filterByType and not the index. There are types that are not used and the ordering in the combobox could easily change. The TransactionFilterProxy::TYPE(type) will not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good idea, i've reworked this a bit to address this

@fuzzbawl
Copy link

Addressed the @random-zebra nit, and implemented the @fuzzbawl proposal to also persist the ordering selection.

I keep getting emails because I was mentioned here. I think you meant @Fuzzbawls instead.

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK 96a4232

Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK 96a4232 and merging...

@random-zebra random-zebra merged commit e7dd094 into PIVX-Project:master Jul 22, 2020
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Jul 22, 2020
furszy added a commit that referenced this pull request Jul 24, 2020
dd22385 [GUI] Update translations from transifex (Fuzzbawls)
919e060 Solving old, not loaded at startup, transactions notification issue. (furszy)
943d84b NU name words separated by underscore instead of white space. (furszy)
fc042f1 Fixing UpgradeIndex position issue. (furszy)
531673c Customize network upgrades activation height for regtest at the startup. (furszy)
1337685 Sapling activation height for regtest (furszy)
15b1052 Miner: Unused reserveKey cleanup (furszy)
566b6fc [GUI] Load persisted transaction filter/sort during start (V3) (Mrs-X)
5d22583 [GUI] Recognize key event for clearing console (Fuzzbawls)
77880e0 GUI: Settings console message moved to read only. (furszy)
6b63591 options model unused signals cleanup (furszy)
b9e3859 GUI: Fixing settings display lang initialization (furszy)
b434cbf Transaction primitive unused methods cleanup. (furszy)
7f9037f missing validation_zerocoin_legacy added to CMakeLists.txt (furszy)
22bfc12 Move-only: ATMP zerocoin check moved to its own legacy file. (furszy)
24d9ac4 Move-only: DataBaseAccChecksum to zerocoin validation legacy. (furszy)
fb2993d Move-only: Move Zerocoin tx disconnection from DisconnectBlock to its own legacy file method. (furszy)
eaacd29 Wallet::AddToWallet if walletdb is null, create one and write the tx. (furszy)
609407d Split CWallet::AddToWallet into AddToWallet and LoadToWallet. (furszy)
9aacb38 Prevent multiple calls to CWallet::AvailableCoins (furszy)
c0e9b13 Fix insanity of CWalletDB::WriteTx and CWalletTx::WriteToDisk (furszy)

Pull request description:

  backports the following PRs that have been merged since the `4.2` branch-off:

  #1717
  #1733
  #1736
  #1742
  #1741
  #1744
  #1287
  #1751
  #1747
  #1749
  #1763

ACKs for top commit:
  furszy:
    utACK dd22385 , ready for 4.2 :) .

Tree-SHA512: 38dd47320b8a7de77879240c36dec7588671fc09bc7d4f0be573fb6a33d6d243d4adbf4451bd4b4da9c046fab058c1cb0eb2ec2d97052b4d1171ac959ae5a71f
@Fuzzbawls Fuzzbawls removed the Needs Backport Placeholder tag for anything needing a backport to prior version branches label Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants