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

Add live data to wallet sync details #469

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

oshorefueled
Copy link
Contributor

This PR closes #447. It adds live wallet sync details to the overview page.

Screenshot 2019-12-26 at 10 28 45 AM

Screenshot 2019-12-26 at 10 27 36 AM

Copy link
Contributor

@metaclips metaclips left a comment

Choose a reason for hiding this comment

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

image
There seems to be spacing between trailing and leading amount text, you should check how send page confirmation window implements amount texts https://github.com/raedahgroup/godcr/blob/0dc5f3cac669f276f1244f061976bdcf17212380/fyne/pages/handler/sendpagehandler/confirmationwindow.go#L47

image
We should add padding

image
Overview text should be titled
image
All header text should be titled.
image
Amount table contents should be right aligned
image
table contents should be centered

Copy link
Contributor

@metaclips metaclips left a comment

Choose a reason for hiding this comment

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

You omitted wallet sync connect/disconnect button on overview page.

fyne/pages/handler/overview.go Show resolved Hide resolved
fyne/pages/overview.go Outdated Show resolved Hide resolved
fyne/pages/overview.go Outdated Show resolved Hide resolved
fyne/pages/handler/overview.go Outdated Show resolved Hide resolved
fyne/pages/display.go Show resolved Hide resolved
@oshorefueled
Copy link
Contributor Author

image
There seems to be spacing between trailing and leading amount text, you should check how send page confirmation window implements amount texts

https://github.com/raedahgroup/godcr/blob/0dc5f3cac669f276f1244f061976bdcf17212380/fyne/pages/handler/sendpagehandler/confirmationwindow.go#L47

image
We should add padding

image
Overview text should be titled
image
All header text should be titled.
image
Amount table contents should be right aligned
image
table contents should be centered

Kindly clarify what you mean by titled.

@metaclips
Copy link
Contributor

image
There seems to be spacing between trailing and leading amount text, you should check how send page confirmation window implements amount texts
https://github.com/raedahgroup/godcr/blob/0dc5f3cac669f276f1244f061976bdcf17212380/fyne/pages/handler/sendpagehandler/confirmationwindow.go#L47

image
We should add padding
image
Overview text should be titled
image
All header text should be titled.
image
Amount table contents should be right aligned
image
table contents should be centered

Kindly clarify what you mean by titled.

First character should be upper case

moved update logic to handlers

added more live data and cleaned up overview handler
moved sync status calls to separate goroutine

removed block sync status when sync is complete

fix balance bug and add dynamic sync status
clean up UI and handler code

add indentation to status text and padding to page
fix wallet details bug

added delay for table implementation

fix overview render delay bug
add padding to wallet boxes and fix on sync button

fix balance spacing and transactions alignment

add capitalization to overview and transactions headers
@oshorefueled
Copy link
Contributor Author

Screenshot 2020-01-07 at 12 45 25 PM

Copy link
Contributor

@metaclips metaclips left a comment

Choose a reason for hiding this comment

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

UI Changes

Screenshot 2020-01-08 at 16 51 39

  • We need to remove horizontal scroll, confirmed/pending text needs to be labeled

  • Bottom padding doesn't seem right, can we add decred logo at the top of overview text?

  • Table looks off, can we instead start with Direction Icon, Date, Amount, Fee, Status ("Left Aligned") check below image for reference and also I guess Direction is same thing as # on the table

Screenshot 2020-01-08 at 16 51 39

  • When sync completes and objects are hidden, the window size isn't calibrated and table centered

Screenshot 2020-01-08 at 16 16 00

  • When wallet is disconnected sync steps and wallet info bar are shown
    Color when wallet is synced, syncing and not synced seems off
    Texts in bordered wallet info can be enlarged so as to be clear and border can be padded

Screenshot 2020-01-08 at 16 16 11

  • while syncing it doesn't report how many minutes left for sync to be done but instead there's a timer and also the scroller housing wallet info moves back to the starting if scrolled

Screenshot 2020-01-08 at 17 55 41

Copy link
Contributor

@metaclips metaclips left a comment

Choose a reason for hiding this comment

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

When I created a new wallet and tried canceling sync, the window stalls and I have to force close the app also, progress bar isn't updated while syncing
sync/waiting for sync seems not to be accurate
Screenshot 2020-01-08 at 19 06 23
show/hide details button is missing
Progress bar should be updated for all steps
Lastly can we do away with the goroutines used in overview.go? I still see no reason for this, we initialize all dynamic objects then pass them to the txnotifier which should be capable of handling all dynamic objects

Comment on lines +26 to +54
Synced bool
Syncing bool
SyncProgress float64
ConnectedPeers int32
Steps int32
hideSyncDetails bool

Container fyne.CanvasObject
TransactionsContainer *fyne.Container
Balance []*canvas.Text
Transactions []dcrlibwallet.Transaction
PageBoxes fyne.CanvasObject
SyncStatusWidget *canvas.Text
TimeLeftWidget *widget.Label
BlockHeightTime *widget.Label
ProgressBar *widget.ProgressBar
BlockStatus *fyne.Container
BlockStatusSynced fyne.CanvasObject
BlockStatusSyncing fyne.CanvasObject
Table *widgets.Table
TableHeader *widget.Box
ConnectedPeersWidget *widget.Label
SyncStepWidget *widget.Label
BlockHeadersWidget *widget.Label
WalletSyncInfo *fyne.Container
Scroll *widget.ScrollContainer
SyncButton *widget.Button

StepsChannel chan int32
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to make all variables exportable? can we only make variables that are dynamic exportable? we should try our best to bring about encapsulation of variables and functions/methods
This are the variables that are needed
Sync label
Table widget
Time left label
Last time of block fetched
Balance label (we just pass the *fyne.Container object and then if we need to dynamically change this we can pass the values to the struct field "Objects" to dynamically change the balance label)
Button to enable/disable syncing
Steps label and Header/address discovery label
Connected peer count label
Block count label
contents in bordered wallet info (synced, block header fetched and syncing progress label) we can pass them as an array of *canvas.Text

You only need to make the functions/methods that initializes overview page and that are passed to txnotifier.go exportable in the handler package so that all other helper functions/methods in handler package are private to pages package bringing about encapsulation also, functions to be passed to txnotifier can be moved to the handler package.

func (handler *OverviewHandler) SyncTrigger(wallet *dcrlibwallet.MultiWallet) {
var notifyTitle, notifyMessage string
synced, syncing := wallet.IsSynced(), wallet.IsSyncing()
if wallet.ConnectedPeers() > 0 && (synced || syncing) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there's no internet connection and connected peers is 0? I really dont think this conditions are necessary, only if wallet.IsSyncing or not should do

fyne/pages/display.go Show resolved Hide resolved
@oluwandabira oluwandabira added fyne godcr-fyne enhancement New feature or request labels Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fyne godcr-fyne
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add live block sync status
3 participants