Hi Randy, here’s a code review.
Thanks for the BSD license.
I prefer the look of a minimized import path, I would have put the title
library at the top level (github.com/goki/ki).
To me the README doesn’t balance text and code examples well enough, I’d
like to see more example uses and less text. In package ki I’d hope the
library does enough work to not require so much README.
I think ki isn’t a bad package name after reading what it means but seeing
the package used in app code won’t show the point as obviously as an
English word for some people. tree.New() to me says “new data structure
var” while ki.New() says “I have to read the docs now”.
(when I say 'app' I mean a program that uses your library or a program
written to compile with a Go compiler; an application of the library, an
application of the Go programming language)
The Ki interface is a code smell to me. Usually I interpret library
interfaces as saying “the app provides an implementation of the interface
and the library provides shared logic that uses the interface methods” or
in this case I expect the ki lib will provide varying data structures and
behaviors that implement the ki var. Just reading down to line 42 of ki.go
I feel like this isn’t very Go-like.
You’re getting into generics territory with this:
func NewOfType(typ reflect.Type) Ki {
My view is idiomatic Go code tends to avoid this kind of thing due to
maintenance and readability burden. I haven’t used your lib but I am
concerned about it not being a big win over a per-app implementation. I can
see you’ve done a lot of work to make generics work.
In type Node I would consider embedding Props.
I haven’t looked at all of the methods but do the methods on Node need to
be to a pointer?
func (n *Node) Fields() []uintptr {
Seeing uintptr in a public method is a code smell to me.
In type Deleted consider maybe embedding sync.Mutex.
// fmt.Printf("finding path: %v\n", k.Path)
Instead of comments you may want to consider this pattern:
const debug = false
…
if debug {
fmt.Printf("finding path: %v\n", k.Path)
}
I think this library would be a good study for the Go 2
(https://blog.golang.org/toward-go2) generics effort, if you have time
consider writing an experience report and posting it:
https://github.com/golang/go/issues/15292
Perhaps consider embedding ki.Signal in gi.Action, and MakeMenuFunc,
ki.Signal, *Icon, and ButtonStates in ButtonBase, some fields in ColorView,
Dialog, FillStyle, FontStyle, LayoutStyle, LayoutData, Layout, MapView,
MapViewInline, Node2DBase, Paint, ImagePaintServer, SliceView,
SliceViewInline, SliderBase, SplitView, StrokeStyle, StructView,
StructViewInline, BorderStyle, ShadowStyle, Style, TabView, TextStyle,
TextField, SpinBox, ComboBox, TreeView, ValueViewBase, Viewport2D, Window.
I like to avoid any unnecessary field symbols.
These kinds of structs are a code smell to me:
type ColorValueView struct {
ValueViewBase
}
Why a Base type? Can your types be simpler?
type Node2D and type ValueView seem like other possible overuses of
interface to me. The size of type PaintServer is closer to what I’d expect
with an interface, and Labeler seems idiomatic.
I like the screenshot.
// check for interface implementation
var _ Node2D = &Line{}
I don’t like this pattern.
Given the amount of code and project maturity I wouldn’t expect you to make
a lot of changes, but I do think it could have been built with a stronger
resilience to change. Thanks for sharing here.
Matt
On Friday, May 4, 2018 at 5:39:35 AM UTC-5, Randall O'Reilly wrote:
>
> https://github.com/goki/goki — key demo in:
> https://github.com/goki/goki/tree/master/gi/examples/widgets
>
> This is the first release of a new Go framework built around the Tree as a
> core data structure (Ki = Tree in Japanese), which includes as its first
> application a fully-native Go GUI (built on top of a modified version of
> the Shiny OS-specific backend drivers, supporting Mac, Linux, and Windows
> so far).
>
> Building on the central idea in Go that having a few powerful
> data-structures is essential for making many problems easier to solve, the
> GoKi trees are an attempt to provide a powerful tree structure that can
> support things like scene graphs, DOM’s, parsing trees, etc.
>
> The GoGi graphical interface system is a kind of “proof is in the pudding”
> test, which weighs in at under 20k LOC and provides a reasonably
> full-featured GUI — with a bit more work it should be able to do most of
> the stuff you can do in Qt, and already includes a (self) reflection-driven
> GUI designer.
>
> The overall design is an attempt to integrate existing standards and
> conventions from widely-used frameworks, including Qt (overall widget
> design), HTML / CSS (styling), and SVG (rendering). Rendering in SVG is
> directly supported by the GoGi 2D scenegraph, with enhanced functionality
> for interactive GUI's. This 2D framework also integrates with a (planned)
> 3D scenegraph, to support interesting combinations of these frameworks.
> Currently GoGi is focused on desktop systems, but nothing prevents
> adaptation to mobile.
>
> Right now the rendering is based off of a modified version of
> https://github.com/fogleman/gg, but I’m very interested in integrating
> the new rasterx system that Steven Wiley recently announced.
>
> I’d be very interested in people’s impressions, suggestions, etc, and
> welcome all interested contributors (there’s certainly much more to do) —
> it would be great if this could provide the start for a widely-supported
> Go-native GUI framework! This was my first Go project after many years in
> C++ / Qt land, and I’m excited to join the community, and have really been
> impressed with the language and ecosystem etc. The contrast in complexity
> and build time between Qt and GoGi is really striking, and has kept me
> going despite the huge amount of effort it took to get this new project off
> the ground.. Cheers,
>
> - Randy
>
>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
