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] Rework of overview page #479

Merged
merged 2 commits into from Jan 16, 2018

Conversation

Projects
None yet
7 participants
@Warrows
Collaborator

Warrows commented Jan 6, 2018

Here is a proposal for some modifications on the overview page. Thanks to @Mrs-X work it got some really nice improvements but I believe it still holds a few flaws. So here are my proposed changes:

-First, put the combined balance on top. Users want to know how much they have, and how much they can spend right away. Stop.
-For the same reason, remove emphasis from piv balance and put it to combined.
-Move PIVX/zPIV percentages to the areas labels. (piv one was in front of unlocked which was technically false)
-Change sections names for more consistency (plurals, zPIV instead of zerocoin)
-Move locked to the PIVX balance instead of combined (you can't lock zPIV, can you ?)
-Remove redundant informations like zPIV and PIVX balances in both their own areas and the combined balance area.
-Reverse zPIV balances order to be consistent with the two other sections.
-Have a clear available/total policy for each section.

Here is a small before/after comparison.
before
after

I believe all previously available information are still in here.

@presstab

This comment has been minimized.

Show comment
Hide comment
@presstab

presstab Jan 6, 2018

Collaborator

I like the concept here. utACK. Need to confirm that this reflects all balances correctly.

Collaborator

presstab commented Jan 6, 2018

I like the concept here. utACK. Need to confirm that this reflects all balances correctly.

@Mrs-X

Mrs-X approved these changes Jan 6, 2018 edited

I like the concept as well, and I guess nobody will really miss the "Unlocked" balance.
I wonder how possible Watched-Balances will look like, but we'll find out soon enough.

ACK 32fdefd

@Fuzzbawls Fuzzbawls added the GUI label Jan 7, 2018

@Fuzzbawls

Some style changes noted.

Overall Concept ACK, needs some thorough testing, especially with watch-only cases.

Show outdated Hide outdated src/qt/forms/overviewpage.ui Outdated
Show outdated Hide outdated src/qt/forms/overviewpage.ui Outdated
Show outdated Hide outdated src/qt/forms/overviewpage.ui Outdated
Show outdated Hide outdated src/qt/forms/overviewpage.ui Outdated
Show outdated Hide outdated src/qt/forms/overviewpage.ui Outdated
Show outdated Hide outdated src/qt/overviewpage.cpp Outdated
Show outdated Hide outdated src/qt/overviewpage.cpp Outdated
Show outdated Hide outdated src/qt/overviewpage.cpp Outdated
Show outdated Hide outdated src/qt/overviewpage.cpp Outdated
Show outdated Hide outdated src/qt/forms/overviewpage.ui Outdated
@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 7, 2018

Collaborator

Thanks to thorough review by @Fuzzbawls I got to make some changes.
I added a few "notr" tags which where absent in the original file.
Fixed one or two editor fizzles.
And I rolled back some coding style changes regarding whitespaces. But I am left wondering if and where we have coding style guidelines.

Collaborator

Warrows commented Jan 7, 2018

Thanks to thorough review by @Fuzzbawls I got to make some changes.
I added a few "notr" tags which where absent in the original file.
Fixed one or two editor fizzles.
And I rolled back some coding style changes regarding whitespaces. But I am left wondering if and where we have coding style guidelines.

@Mrs-X

This comment has been minimized.

Show comment
Hide comment
@Mrs-X

Mrs-X Jan 7, 2018

Collaborator

where we have coding style guidelines.

Not complete, but gets you started:
https://github.com/PIVX-Project/PIVX/blob/master/doc/developer-notes.md

Collaborator

Mrs-X commented Jan 7, 2018

where we have coding style guidelines.

Not complete, but gets you started:
https://github.com/PIVX-Project/PIVX/blob/master/doc/developer-notes.md

@veramispotato

This comment has been minimized.

Show comment
Hide comment
@veramispotato

veramispotato Jan 8, 2018

Much cleaner, thanks! In last image,

Suggest auto-hiding the following if they are 0: Pending, Immature, Unconfimed, Locked.

Suggest removing Total and Available if they equal the same number, and only show the number.

Correction: PIVX Balances should be PIV Balance, or suggest changing to PIV
Correction: zPIV Balances should be zPIV Balance, or suggest changing to zPIV

veramispotato commented Jan 8, 2018

Much cleaner, thanks! In last image,

Suggest auto-hiding the following if they are 0: Pending, Immature, Unconfimed, Locked.

Suggest removing Total and Available if they equal the same number, and only show the number.

Correction: PIVX Balances should be PIV Balance, or suggest changing to PIV
Correction: zPIV Balances should be zPIV Balance, or suggest changing to zPIV

@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 8, 2018

Collaborator

So yeah 7a8ad53 Implements changes suggested by @wholewheatbiscuits
It also hides PIV and zPIV areas when there are none.
I also made sure that watch-only addresses are working as pointed out by @Fuzzbawls
Here are the latest screenshots:
When opening a new (empty) wallet
image
When using it like a "normal" user (aka all operations are complete)
image
And finally when you like fancy stuff like masternodes and watch-only addresses
image
Obviously no configuration needed whatsoever and everything you own is always visible. I am just hiding empty and/or redundant balances.

Collaborator

Warrows commented Jan 8, 2018

So yeah 7a8ad53 Implements changes suggested by @wholewheatbiscuits
It also hides PIV and zPIV areas when there are none.
I also made sure that watch-only addresses are working as pointed out by @Fuzzbawls
Here are the latest screenshots:
When opening a new (empty) wallet
image
When using it like a "normal" user (aka all operations are complete)
image
And finally when you like fancy stuff like masternodes and watch-only addresses
image
Obviously no configuration needed whatsoever and everything you own is always visible. I am just hiding empty and/or redundant balances.

@turnkit

This comment has been minimized.

Show comment
Hide comment
@turnkit

turnkit Jan 8, 2018

Warrows - great improvement.
Three concerns:

  1. tpiv - is this for "test network pivs" or "thousand pivs" or "total pivs?" Either way, it's confusing. (And hopefully mPIVs and uPIVs eventually can get clever arbitrary names like milliPIVs, microPIVs, 'Doriens', 'Warrows', 'PIVpennies' or 'PIVbits', etc. so these units show up in more human friendly names.)
  2. I'd suggest the "simple view" just show two numbers: avail. PIVs & avail. zPIVs.
  3. when there is not "detailed view" to be seen, still allow the user a way to twiddle/click an arrowhead to see all the "0.00" entries and their descriptions -- but that by default when they are all zero, that arrow is up/closed / view is not seen.

turnkit commented Jan 8, 2018

Warrows - great improvement.
Three concerns:

  1. tpiv - is this for "test network pivs" or "thousand pivs" or "total pivs?" Either way, it's confusing. (And hopefully mPIVs and uPIVs eventually can get clever arbitrary names like milliPIVs, microPIVs, 'Doriens', 'Warrows', 'PIVpennies' or 'PIVbits', etc. so these units show up in more human friendly names.)
  2. I'd suggest the "simple view" just show two numbers: avail. PIVs & avail. zPIVs.
  3. when there is not "detailed view" to be seen, still allow the user a way to twiddle/click an arrowhead to see all the "0.00" entries and their descriptions -- but that by default when they are all zero, that arrow is up/closed / view is not seen.
@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 8, 2018

Collaborator

@turnkit Thanks for your valuable input.

  1. tPIV stands for testnetPIV, no worries. About units, it's an interesting question but not to be solved here. I believe it's a community wide question which will need further discussions.
  2. It might be unclear but there is no "simple view" here. It's one and only one panel which shows only numbers above 0. So if you have no pending operation and no locked up PIV in any way you get what's seen in the second image: Total (which happen to also be available), PIV and zPIV.
  3. I am not sure there is a point in showing all the empty balances. If you are a new user you most likely don't care about them and if you are more a advanced one you probably already know about them and don't want it to cluster your view for nothing.

Anyway you'll see them soon enough if you move funds around. And if they don't show long it's that the network is so fast you don't need to know about them.

Collaborator

Warrows commented Jan 8, 2018

@turnkit Thanks for your valuable input.

  1. tPIV stands for testnetPIV, no worries. About units, it's an interesting question but not to be solved here. I believe it's a community wide question which will need further discussions.
  2. It might be unclear but there is no "simple view" here. It's one and only one panel which shows only numbers above 0. So if you have no pending operation and no locked up PIV in any way you get what's seen in the second image: Total (which happen to also be available), PIV and zPIV.
  3. I am not sure there is a point in showing all the empty balances. If you are a new user you most likely don't care about them and if you are more a advanced one you probably already know about them and don't want it to cluster your view for nothing.

Anyway you'll see them soon enough if you move funds around. And if they don't show long it's that the network is so fast you don't need to know about them.

@turnkit

This comment has been minimized.

Show comment
Hide comment
@turnkit

turnkit Jan 8, 2018

turnkit commented Jan 8, 2018

@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 8, 2018

Collaborator

@turnkit I absolutely understand your feeling that the user must be king and I agree with it. The thing is I don't think the average user will open the advanced view if he doesn't need it. And even if he did, there is no way we could stuff enough information in the software. If you want to understand cryptocurrencies you need to go out and educate yourself about them. As much as we would want it, we can't give a full course about blockchain and zPIV minting in-wallet.
And in the case you just want to use the wallet without bothering too much about its internal workings, the fact that the locked coins show up when they are locked, with the much needed tooltip explaining why/how they are locked seems good enough to me.
You say yourself, it becomes useful to read about them when this situations happen (and they will happen, and the balances will be there, no click needed). Before their coins are locked, most users don't want to know what can happen to them. And those who want will learn on our website or youtube.

Collaborator

Warrows commented Jan 8, 2018

@turnkit I absolutely understand your feeling that the user must be king and I agree with it. The thing is I don't think the average user will open the advanced view if he doesn't need it. And even if he did, there is no way we could stuff enough information in the software. If you want to understand cryptocurrencies you need to go out and educate yourself about them. As much as we would want it, we can't give a full course about blockchain and zPIV minting in-wallet.
And in the case you just want to use the wallet without bothering too much about its internal workings, the fact that the locked coins show up when they are locked, with the much needed tooltip explaining why/how they are locked seems good enough to me.
You say yourself, it becomes useful to read about them when this situations happen (and they will happen, and the balances will be there, no click needed). Before their coins are locked, most users don't want to know what can happen to them. And those who want will learn on our website or youtube.

@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 9, 2018

Collaborator

@presstab Thanks, totally agree. I just followed the existing pattern here, now it's done for all the same looking functions.

Collaborator

Warrows commented Jan 9, 2018

@presstab Thanks, totally agree. I just followed the existing pattern here, now it's done for all the same looking functions.

@Mrs-X

This comment has been minimized.

Show comment
Hide comment
@Mrs-X

Mrs-X Jan 9, 2018

Collaborator

I'm not 100% sure if completely hiding elements when they are empty is support-wise a good idea.
We can't use that space anyway...

Have you ever tried to guide someone via phone through a problem who sees a different UI?

Collaborator

Mrs-X commented Jan 9, 2018

I'm not 100% sure if completely hiding elements when they are empty is support-wise a good idea.
We can't use that space anyway...

Have you ever tried to guide someone via phone through a problem who sees a different UI?

@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 9, 2018

Collaborator

@Mrs-X I can't say I am 100% sure it's the best idea. But I really like the cleanliness it brings.
I am not sure either about the different UI thing, it's not like hiding a button or putting things elsewhere depending on OS. It's more about putting away empty information. If you try to help someone, you know it's either there or 0.
Maybe let's try to get some support people input on this.
If there is a consensus I can rollback the hiding thing. But I think it does not make much sense to have it for some lines (was existing for pending, and maybe immature in PIV balance) and not others.

Collaborator

Warrows commented Jan 9, 2018

@Mrs-X I can't say I am 100% sure it's the best idea. But I really like the cleanliness it brings.
I am not sure either about the different UI thing, it's not like hiding a button or putting things elsewhere depending on OS. It's more about putting away empty information. If you try to help someone, you know it's either there or 0.
Maybe let's try to get some support people input on this.
If there is a consensus I can rollback the hiding thing. But I think it does not make much sense to have it for some lines (was existing for pending, and maybe immature in PIV balance) and not others.

@turnkit

This comment has been minimized.

Show comment
Hide comment
@turnkit

turnkit Jan 9, 2018

turnkit commented Jan 9, 2018

@turnkit

This comment has been minimized.

Show comment
Hide comment
@turnkit

turnkit Jan 9, 2018

turnkit commented Jan 9, 2018

@Mrs-X

This comment has been minimized.

Show comment
Hide comment
@Mrs-X

Mrs-X Jan 10, 2018

Collaborator

If there is a consensus I can rollback the hiding thing.

@Warrows What about a new setting "Hide zero balances" to make this optional?
Could be used at other places as well.

Collaborator

Mrs-X commented Jan 10, 2018

If there is a consensus I can rollback the hiding thing.

@Warrows What about a new setting "Hide zero balances" to make this optional?
Could be used at other places as well.

@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 12, 2018

Collaborator

@Mrs-X We can add a setting of course. But the point being to have a cleaner overview for newcomers the auto-hiding should be opt-out. Would you agree with that ?
Because in my view it's not a cosmetic change for power users but an effort to make the wallet more accessible.

Collaborator

Warrows commented Jan 12, 2018

@Mrs-X We can add a setting of course. But the point being to have a cleaner overview for newcomers the auto-hiding should be opt-out. Would you agree with that ?
Because in my view it's not a cosmetic change for power users but an effort to make the wallet more accessible.

@Mrs-X

This comment has been minimized.

Show comment
Hide comment
@Mrs-X

Mrs-X Jan 13, 2018

Collaborator

@Warrows I just played around with different options.
I think your first screenshot (only Combined Balance) goes a bit too far, the user, even when he's a beginner, should see that something like PIV and zPIV exist, even when one or both them are zero.

So if a more minimalistic overview should be the default I'd like to see the second screenshot as default one (this one:
https://user-images.githubusercontent.com/835098/34684650-1e1cae60-f4a6-11e7-9769-b4b63fb2864a.png).

If more balances are available they should be automatically visible of course, and users who always want to see everything get a checkbox in the option menu.

Collaborator

Mrs-X commented Jan 13, 2018

@Warrows I just played around with different options.
I think your first screenshot (only Combined Balance) goes a bit too far, the user, even when he's a beginner, should see that something like PIV and zPIV exist, even when one or both them are zero.

So if a more minimalistic overview should be the default I'd like to see the second screenshot as default one (this one:
https://user-images.githubusercontent.com/835098/34684650-1e1cae60-f4a6-11e7-9769-b4b63fb2864a.png).

If more balances are available they should be automatically visible of course, and users who always want to see everything get a checkbox in the option menu.

@veramispotato

This comment has been minimized.

Show comment
Hide comment
@veramispotato

veramispotato Jan 13, 2018

veramispotato commented Jan 13, 2018

@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 13, 2018

Collaborator

@Mrs-X You are absolutely right, newcomers must see that there are two separate balances. Maybe hiding everything was a bit extreme.
I implemented the setting as opt-out (checkbox is checked by default, therefore default view is second image) in f751e12
image
And put back the two main panels as always visible in 778d9f7
image

Collaborator

Warrows commented Jan 13, 2018

@Mrs-X You are absolutely right, newcomers must see that there are two separate balances. Maybe hiding everything was a bit extreme.
I implemented the setting as opt-out (checkbox is checked by default, therefore default view is second image) in f751e12
image
And put back the two main panels as always visible in 778d9f7
image

@Mrs-X

Mrs-X requested changes Jan 14, 2018 edited

Function wise ACK, but you've messed up the resizing of the screen, see screenshots below.
This needs to be fixed for both the minimal view (if possible, might be tricky to do) and the full/original view.

Before:
resize-orig

After your change:
resize-new

@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 14, 2018

Collaborator

I think I know exactly what the problem is, I am going to push that tomorrow. Do you want me to rebase and squash some commits by the way ?

Collaborator

Warrows commented Jan 14, 2018

I think I know exactly what the problem is, I am going to push that tomorrow. Do you want me to rebase and squash some commits by the way ?

@Warrows

This comment has been minimized.

Show comment
Hide comment
@Warrows

Warrows Jan 15, 2018

Collaborator

@Mrs-X I made the resize work like before. I personally prefer the "fixed size" version. But that's not the point of this PR.

Collaborator

Warrows commented Jan 15, 2018

@Mrs-X I made the resize work like before. I personally prefer the "fixed size" version. But that's not the point of this PR.

@Mrs-X

This comment has been minimized.

Show comment
Hide comment
@Mrs-X

Mrs-X Jan 15, 2018

Collaborator

I personally prefer the "fixed size" version.

Lots of people specifically asked to have the resizing like this, and it makes sense if we want to make use of bigger fonts to support 4k or 8k displays somewhere in the future.

I'll test later, we should do squashing when everything is ACKed.

Collaborator

Mrs-X commented Jan 15, 2018

I personally prefer the "fixed size" version.

Lots of people specifically asked to have the resizing like this, and it makes sense if we want to make use of bigger fonts to support 4k or 8k displays somewhere in the future.

I'll test later, we should do squashing when everything is ACKed.

@Mrs-X

Mrs-X approved these changes Jan 15, 2018

@Mrs-X

This comment has been minimized.

Show comment
Hide comment
@Mrs-X

Mrs-X Jan 15, 2018

Collaborator

ACK 👍

When it's squashed and @Fuzzbawls approves his requested changes we'll merge.

Collaborator

Mrs-X commented Jan 15, 2018

ACK 👍

When it's squashed and @Fuzzbawls approves his requested changes we'll merge.

[UI] Rework of overview page
Put combined balance on top
Removed redundent informations
Fixed some inconsistencies
Rollback styles changes and errors
Hide balances when empty
Hide available balances when equal total
Fix a few things
Add setting to show or hide empty balances in overview
Always show PIV/zPIV panels. Hide percentages when balance is 0
Rollback changes in overview page resizing
@Mrs-X

This comment has been minimized.

Show comment
Hide comment
@Mrs-X

Mrs-X Jan 15, 2018

Collaborator

ACK b5f420b

Collaborator

Mrs-X commented Jan 15, 2018

ACK b5f420b

@Fuzzbawls

This comment has been minimized.

Show comment
Hide comment
@Fuzzbawls

Fuzzbawls Jan 16, 2018

Collaborator

ACK b5f420b

Collaborator

Fuzzbawls commented Jan 16, 2018

ACK b5f420b

@Fuzzbawls Fuzzbawls added this to the 3.1.0 milestone Jan 16, 2018

@Fuzzbawls Fuzzbawls added the Usability label Jan 16, 2018

@Fuzzbawls Fuzzbawls merged commit b5f420b into PIVX-Project:master Jan 16, 2018

1 check passed

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

Fuzzbawls added a commit that referenced this pull request Jan 16, 2018

Merge #479: [UI] Rework of overview page
b5f420b [Refactor] Moved wallet balance computation from header to source file (warrows)
2cd94c7 [UI] Rework of overview page (warrows)

Tree-SHA512: 5adecaefe3ca3e60594ecd3b48e65def15ea458a6a1d49b5a38db8f91fd95da0476127a2bd4bfdcfed2c1d6539f01652d8400d4cc30150c2cf68b0f7f21cb903
@Sieres

This comment has been minimized.

Show comment
Hide comment
@Sieres

Sieres Apr 1, 2018

ACK for the hide zero balances. Balance sections do no collapse like @Warrows examples

Sieres commented Apr 1, 2018

ACK for the hide zero balances. Balance sections do no collapse like @Warrows examples

@Fuzzbawls Fuzzbawls changed the title from [UI] Rework of overview page to [Qt] Rework of overview page Apr 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment