-
Notifications
You must be signed in to change notification settings - Fork 92
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
client: application skeleton #100
Conversation
Looks good to me! |
Good foundation for the client. |
The dexclient binary actually snuck into master with some earlier client stuff. This PR removes it. |
Oh, whoops. I read the diff backwards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. The left side for me, one character is cut off. I guess could use a way to resize... or the dots, like "Accounts & Wa..."
Also, building with the --race flag and going to either web or rpc server and pressing enter twice finds two data races.
And, the white text on a grey background is difficult for me to see everywhere...
if _, ok := err.(*os.PathError); !ok { | ||
fmt.Fprintln(os.Stderr, err) | ||
parser.WriteHelp(os.Stderr) | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use go's new error unwrapping here.
https://golang.org/pkg/errors/
if errors.Is(err, os.PathError) {
fmt.Fprintln(os.Stderr, err)
parser.WriteHelp(os.Stderr)
return nil, err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.PathError
is a type, not a sentinel, so I think you would need to use errors.As
here, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dajohi does this work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would be As
, but I'm comfortable with it the old way too.
var welcomeMessage = "Welcome to Decred DEX. Use [#838ac7]Up[white] and " + | ||
"[#838ac7]Down[white] arrows and then press [#838ac7]Enter[white] to select" + | ||
" a new view. The [#838ac7]Escape[white] key will usually toggle the focus" + | ||
" between the menu and the currently selected view. When the selected view" + | ||
" is focused, most navigation can be done with your the [#838ac7]Left" + | ||
"[white] and [#838ac7]Right[white] arrows, or alternatively [#838ac7]Tab" + | ||
"[white] and [#838ac7]Shift+Tab[white]. Use [#838ac7]Escape[white] to remove" + | ||
" focus from a form element." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The white on grey is hard for me to see...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White on grey? Do you mean the purple parts? The white text with default background is default console coloring, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the first pic I uploaded you can see what I see by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh weird. I'll look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dcrdex/client/cmd/dexc/ui/widgets.go
Line 473 in 47ece4c
tview.Styles.PrimitiveBackgroundColor = backgroundColor |
See if that works for you when you get a chance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What interesting is that the inline-colored (1)
next to Notifications
is colored yellow instead of orange, and the inline key specifier text in the application log text is not purple. So it looks like some color incompatibility problems in general. Maybe you could peek around once this is merged and see if you can find better color values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the purple isn't showing through at all for me. Are you using xterm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I didn't realize it was purple, that's another constant you could make...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The color incompatibility is especially odd consider tview is based on tcell, whose readme opens with
Tcell is a Go package that provides a cell based view for text terminals, like xterm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
// DRAFT NOTE: It's a little odd that the Configure function is from the ui | ||
// package. The ui.Config struct is used both here and in ui. Could create a | ||
// types package used by both, but doing it this way works for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see createApp()
in cmd/dexc/ui/widgets.go accessing the package-level cfg
variable. All of config.go could be moved up to cmd/dexc if we make a separate ui.Config struct
that has only what ui.createApp
needs. This would be nice anyway since the Config
struct used by go-flags has fields (and all struct field tags) that are only relevant to package main
. But you're right, this does work for now.
I must've botched a rebase somewhere. I think I've got it back now, but my history might be a little funky. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really slick looking and it all seems to work right in my testing. It takes a little getting used to tabs to go between test boxes and buttons when arrows are intuitive though.
if _, ok := err.(*os.PathError); !ok { | ||
fmt.Fprintln(os.Stderr, err) | ||
parser.WriteHelp(os.Stderr) | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would be As
, but I'm comfortable with it the old way too.
app.QueueUpdate(func() { | ||
log.Errorf("interpolate error: %v", err) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, not too bad after all. 🤦♂️ I missed that app.QueueUpdate
was available here.
This is a basic skeleton for a client app. It enables running an HTTP server or an RPC server. They both use the same client core, from the new core module. The default behavior is to run the servers from a simple but expandable TUI, but they can also be run without the TUI via command-line arguments or configuration file. Placeholder packages for the actual servers are in place, but don't do anything yet. A basic TUI infrastructure is built on tview, with enough code to demonstrate its use, but ultimately no functionality yet. I'm hoping this provides a good framework on which concurrent development can proceed.
This is a basic skeleton for a client app. It enables running an HTTP server or an RPC server. They both use the same client core, from the new core module. The default behavior is to run the servers from a simple but expandable TUI, but they can also be run without the TUI via command-line arguments or configuration file. Placeholder packages for the actual servers are in place, but don't do anything yet. A basic TUI infrastructure is built on
tview
, with enough code to demonstrate its use, but ultimately no functionality yet.I'm hoping this provides a good framework on which concurrent development can proceed. I plan on getting into the web server this week, and maybe @JoeGruffins could continue fiddling with the TUI, or get into the RPC server. Once the web server is up, there will be front-end work to do there too. @dnldd can import and make use of the the order book code in the client core, and then expand from there.