godcr review, May 2021

The following is a high-level review of the godcr UI code, contained in the
`ui` directory of the godcr repository[0]. Note that the review does not cover
other aspects of the repository, such as security concerns or wallet operation.
In particular, the `wallet` directory is not covered.


== Top-level app.Window reference

To maximize testability and to avoid touching global window state by accident,
I recommend restricting access to the app.Window reference to top-level code,
typically the event loop.

The godcr/ui.Window struct contains a reference to the Gio app.Window. I
suggest removing that and add *app.Window parameters to the `Loop` and
`unhandled` methods.

The godcr/ui.Window.refresh methods calls app.Window.Invalidate. I suggest
using op.InvalidateOp and adding a layout.Context or op.Ops parameter to
`refresh`.


== Clipboard access

godcr/ui.Window uses the raw app.Window.ReadClipboard and WriteClipboard
methods for system clipboard access. Unless top-level clipboard access really
is needed, I suggest using the gioui.org/op/clipboard API instead. It contains
tagged ReadOp and WriteOp operations that can be used from any Gio widget,
without access to an app.Window. Gio automatically takes care of
multiplexing multiple clipboard readers and writers.


== layout.Widget vs func(gtx C) D

Running

$ grep "func.*func\(gtx C\) D"
$ grep "var.*func\(gtx C\) D"

reveals multiple uses of `func(gtx C) D` as an explicit type. I suggest using
the clearer `layout.Widget` type instead.


== Brittle strings.Contains checks

Running

$ grep strings.Contains

returns several instances of strings.Contains whose result may change for
unrelated reasons. For example, the godcr/ui.pageCommon.walletAccountsHandler
method contains the following three checks:

if strings.Contains(page.wallAcctSelector.title, "Sending") {
	...
}
if strings.Contains(page.wallAcctSelector.title, "Receiving") {
	...
}
if strings.Contains(page.wallAcctSelector.title, "Purchasing") {
	...
}

The title checks probably works today but may break in the future when, say, the
wallet is localized to other languages.

Some equals comparisons are also suspect, such as the

if windex == 0 && page.wallAcctSelector.sendOption == "Address"

check in pageCommon.walletAccountModalLayout: the sendOption value is
user-visible.

Error message checks such as the one in *pageCommon.walletAccountsHandler are
another instance of this problem:

strings.Contains(err.Error(), "invalid")

I suggest using errors.Is as a more robust replacement. In this case, the error
comes from strconv.ParseFloat where the concrete error type is always
*strconv.NumError.

There are brittle error equals comparisons as well. Example:

if err.Error() == "exists" {
	errText = "Wallet name already exists"
}

== Explicit DPI conversions

$ grep "\.PxPer

returns a few instances of explicit use of the pixel-to-dp scale. The
pageCommon.layoutNavDrawer converts from Dps to pixels:

gtx.Constraints.Min.X = int(gtx.Metric.PxPerDp) * width

However, PxPerDp may not be integral; a better version is

gtx.Constraints.Min.X = int(gtx.Metric.PxPerDp * width)

Even better is to change the type of width, navDrawerWidth,
navDrawerMinimizedWidth to unit.Value, and use

gtx.Constraints.Min.X = gtx.Px(width)

A related issue is the godcr/ui/decredmaterial.pxf function; it is a copy of
gioui.org/unit.Metric.Px except that it special-cases the zero scale. I think
it is better to never construct zero-valued unit.Metrics and use the standard
Px method instead.


== Use of goroutines for UI tasks

This is a common pattern for displaying a modal dialog:

go func() {
	common.modalReceiver <- &modalLoad{
		...
	}
}()

I'm uncomfortable with the use of non-deterministic and racy goroutines for
what is essentially a single-threaded concept: a list of modals waiting to be
displayed. I suggest adding a function for adding a modal to a global list of
modals. If a modal must be displayed as a result of an external event, use a
separate channel for those events and construct the modals in the same
goroutine as the rest of the UI code.



== Smaller issues:

I find the `margin` parameter of the godcr/ui/decredmaterial.Model.Layout
method unclear:

func (m *Modal) Layout(gtx layout.Context, widgets []func(gtx C) D, margin int) layout.Dimensions

as used in the computation

scaled := 3840 / float32(gtx.Constraints.Max.X)
mg := unit.Px(float32(margin) / scaled)


The file "compontents.go" has an extra "t" in its name.


pageCommon methods use pointer receivers, but pageCommon.layoutNavDrawer does
not. Further, functions such as godcr/ui.transactionRow take a pageCommon
value:

func transactionRow(gtx layout.Context, common pageCommon, row TransactionRow) layout.Dimensions

It's more consistent and efficient to use *pageCommon values.


Several layout.Widget wrappers are redundant. Example:

layout.Rigid(func(gtx C) D {
	return leftWidget(gtx)
}),
layout.Flexed(1, func(gtx C) D {
	return layout.E.Layout(gtx, func(gtx C) D {
		return rightWidget(gtx)
	})
}),

reduces to just

layout.Rigid(leftWidget),
layout.Flexed(1, func(gtx C) D {
	return layout.E.Layout(gtx, rightWidget)
}),

In general, a wrapper around a single return statement can often be omitted.


The check

if i != 32 {
	pg.seedEditors.editors[i+1].Editor.Focus()
}

in create_restore_page.go seems suspect; 32 is magic and its purpose unclear.
There's a redundant cast in the same file:

if pg.wal.LoadedWalletsCount() > int32(0)

Same file, a typo:

var msg = "You are about clearing ...

The constants in modal_templates.go should be wrapped in a single const ( )
section.

The godcr/ui.modalLoad.cancel field is an `interface{}` but it seems to me it
could be tightened to `func()`.

pageCommon.fetchExchangeValue returns a nil error when JSON decoding fails. Its
only caller ignores the error, however.

The goroutine in Window.updateStates seems racy. It also seems unnecessary.

The timer callback in pageCommon.Layout sets page.toastLoad.text without
synchronization with the UI goroutine:

page.toastLoad.Timer(time.Second*3, func() {
	page.toastLoad.text = ""
})

godcr/ui/decredmaterial.Badge.Layout contains DPI-dependent constants:

min := image.Point{
	X: 23,
	Y: 23,
}

The godcr/ui/decredmaterial.Editor.editor constructs up to two icons from
vector data per frame if isPassword is true:

icon := mustIcon(widget.NewIcon(icons.ActionVisibilityOff))
if e.Editor.Mask == '*' {
	icon = mustIcon(widget.NewIcon(icons.ActionVisibility))
}

I suggest caching the icons.

The "png" import in godcr/ui/decredmaterial/icon.go is irrelevant; the icon
data is in IconVG format, not PNG. In general, side-effect imports in non-main
packages are discouraged.


[0] https://github.com/planetdecred/godcr