fyne-io / fyne

Cross platform GUI toolkit in Go inspired by Material Design
https://fyne.io/
Other
24.63k stars 1.37k forks source link

Tree Widget constantly calls IsBranch and ChildrenUIs #4339

Open Minoxs opened 11 months ago

Minoxs commented 11 months ago

Checklist

Describe the bug

The default tree widget will constantly call functions like IsBranch and ChildrenUIs. This makes it only usable for a dictionary of strings, since if I use an actual tree structure, or a file tree, this makes the application slow down to a halt due to constantly walking the entire tree.

How to reproduce

  1. Create the widget from the example code
  2. Add it to a running app window
  3. Check the terminal output

Screenshots

No response

Example code

func New() *widget.Tree {
    const root = "/"

    var tree = &widget.Tree{
        Root: root,

        ChildUIDs: func(uid widget.TreeNodeID) (c []widget.TreeNodeID) {
            log.Println("Tree-Get", uid)
            return make([]widget.TreeNodeID, 0)
        },

        IsBranch: func(uid widget.TreeNodeID) (ok bool) {
            log.Println("IsBranch", uid)
            return false
        },

        CreateNode: func(branch bool) (o fyne.CanvasObject) {
            log.Println("CreateNode")
            return widget.NewLabel(root)
        },

        UpdateNode: func(uid widget.TreeNodeID, branch bool, node fyne.CanvasObject) {
            node.(*widget.Label).SetText(uid)
        },
    }

    return tree
}

Fyne version

2.3.5

Go compiler version

1.20

Operating system and version

Windows 10

Additional Information

This can be worked around by using a few caches, but the complexity of that doesn't make sense, plus, the documentation for widget.NewTree suggests that it is a fast implementation.

// NewTree returns a new **performant** tree widget defined by the passed functions.
// childUIDs returns the child TreeNodeIDs of the given node.
// isBranch returns true if the given node is a branch, false if it is a leaf.
// create returns a new template object that can be cached.
// update is used to apply data at specified data location to the passed template CanvasObject.
//
// Since: 1.4

From my limited debugging, it seems that it's due to some underlying renderer calling the tree's MinSize() and/or Layout() which walks the entire tree, even when the entire tree has been walked once and hasn't changed due to any external factors (i.e resized, moved, interacted in any way, etc).

Slightly Offtopic: I noticed quite a few default widgets suffering from similar problems; having very specific use cases. Another example would be the FileDialog, which is a file selection tool that can only be used in a dialog. So if I'm writing an app where it only makes sense to have the file selection embedded on the window, I can't (which is what I'm doing now). I'm forced to roll out my own file selection from scratch basically.

andydotxyz commented 11 months ago

The tree is a performant widget - it will draw only what is visible and re-use content so it can be rendered and scrolled super fast.

What is difficult when the widget does not "own" the data like this is we cannot know when your data changes so caching and updating only on resize will cause the display to become stale.

As you know file operations are slow and so you may need to handle caching in your data layer (as well as change monitoring etc...). If you can think of a way that Tree could be updated to provide better caching of data without running into the troubles described that would be great.

Minoxs commented 11 months ago

Thanks for the quick response!

I can think of a few ways the Tree widget could be changed to improve this behaviour, but it would require changing the API, and even then it might not be useful for some use cases. If the current behaviour - avoid stale data at all costs by constantly walking the tree - is expected and desired then I agree there's not much to be done.

In that case, I would argue that the current Tree widget is doing too much. In my use case, I know that the data will rarely change, and I could just add a refresh button and/or have a bigger refresh interval to make sure the user can see the up to date tree. In other words, I just want to render a tree and have some basic inputs (branch open, branch close, etc).

Currently the only way of doing that would be going down to the lower levels and implementing a new widget from scratch, caching in the data layer, or some other bodge, which is really cumbersome. Thus, I propose a slightly different approach to fixing this: Break down the current Tree widget into BaseTree and FreshTree (or keep it as Tree to avoid breaking changes).

BaseTree can then expose basic methods for tree rendering, like Refresh/Render all of it, Refresh a branch onwards, etc. Does the bare minimum to render a tree, and exposes methods to interact with it. If the developer chooses to use this, then they have to decide how it behaves and implement all of it on the widget layer (doesn't have to rewrite tree rendering! yay).

FreshTree will then extend/use BaseTree and will implement the current behaviour, which won't require any other changes to whoever is currently using it.

This is probably the approach I'll be taking on my project, and if it is within fyne's standards I'd be happy to contribute it here, just let me know. Also, please close this issue if it's falling into "Feature request/proposal" territory or deemed unecessary.

Minoxs commented 11 months ago

On a slight tangent, is there a definition of a widget (and what differs it from a container)? Now that I think about it, I'd probably classify the Tree as a container, and that might be an important difference here.

andydotxyz commented 10 months ago

Currently the only way of doing that would be going down to the lower levels and implementing a new widget from scratch, caching in the data layer, or some other bodge, which is really cumbersome.

Caching in the data layer can already be done without extending tree or creating a new widget... that is possible because of our data separation from the widget.

andydotxyz commented 10 months ago

On a slight tangent, is there a definition of a widget (and what differs it from a container)? Now that I think about it, I'd probably classify the Tree as a container, and that might be an important difference here.

Tree (and List, Table and GridWrap) are collection widgets (as noted in the docs https://developer.fyne.io/ see container, widget, collection division). Containers are just a group of items arranged by a layout. Collections are smart widgets with the re-use ui-caching layer for performance.