Initial implementation of navigation container #5500

sdassow · 2025-02-04T16:46:21Z

Description:

This add a basic navigation container type with back button and title. Sponsored by Fyne Labs.

How it looks like:

Screen.Recording.2025-02-04.at.17.37.28.mov

Example code:

package main

import (
	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/container"
	"fyne.io/fyne/v2/widget"
)

func main() {
	a := app.NewWithID("fyne-navigation")
	w := a.NewWindow("Hello World")

	var nav *container.Navigation
	var view1, view2, view3 *fyne.Container

	view1 = container.NewVBox(
		widget.NewLabel("Foo"),
		widget.NewButton("Push!", func() {
			nav.PushWithTitle(view2, "Bar Title")
		}),
	)

	view2 = container.NewVBox(
		widget.NewLabel("Bar"),
		widget.NewButton("More!", func() {
			nav.PushWithTitle(view3, "Baz Title")
		}),
	)

	view3 = container.NewVBox(
		widget.NewLabel("Baz"),
	)

	nav = container.NewNavigationWithTitle(view1, "NavNavNav")

	w.SetContent(nav)
	w.Resize(fyne.NewSize(400, 400))
	w.ShowAndRun()
}

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

Where applicable:

Public APIs match existing style and have Since: line.

dweymouth · 2025-02-05T11:05:41Z

container/navigation.go

+// Objects can be any CanvasObject, and only the most recent one will be visible.
+//
+// Since: 2.6
+type Navigation struct {


The way this is written it can only be constructed via NewNavigation and not easily extended, or constructed by a struct literal instead, as has currently been a requirement for all builtin Fyne widgets. Most of this struct's contents may be able to move to the renderer instead.

Good point. Have to look from that angle again and compare to other widgets.

This needs more thought than I expected to make sure everything is initialized correctly, but I started making some parts public.

Got it working by adding a setup method and a bunch of checks.

dweymouth · 2025-02-05T11:06:48Z

container/navigation.go

+	"fyne.io/fyne/v2/widget"
+)
+
+// Navigation container is used to provide your application with a control bar and an area for content objects.


If the idea is for this to be the final API, it should probably be called StackNavigation, to distinguish it from other navigation schemes, such as a browser-like queue of past pages that can be navigated back and forward. Unless the idea is to make this single component support multiple navigation schemes in the future

But couldn't you build a browser back/forward with this too? It could be extended to have forward navigation without needing Stack vs not-stack navigation.
Conceptually it feels like the browser is a stack as well?

Is that really the best option? Do we know that all the other navigators will have the same API?

With more and more types in the container package that actually aren't simple containers but technically widgets, I wonder if a "navigation" package would make sense? Tabs would fit in quite well there as well, I'd think. Just an idea to bring up before we commit to anything :)

I'm not sure that tabs fit a mental model of navigation - you're not "going" anywhere...
Container has diversified but the general concept of "you put other widgets in there" mostly holds.

I don't really think you could build a browser-like navigation with this component without just forking it and adding a different API - the browser navigation model is only a bit "stack-like", but it's really more of a list with a pointer that can advance back and forward, and when you push a new page, it deletes the whole "tail" of the history after the current pointer position. (ie back, back, back, new page would remove the last 3 items in the old history)

I'm now considering to change the logic to support an optional "Next" button.

Does that mean the naming works now? If it needs a more explicit name then OK but I'm not convinced that "Stack" or "Simple" add anything meaningful to a developer's understanding of what it is.

I think the naming works if we plan to add (at least eventually) the forward navigation as well. I wonder if users may also confuse this with a "routing" system though, which would be more complex still.

The forward navigation is now in the PR.

I'm happy with this now. If a routing system is to be added that would not be presented as a navigation container in this sense - more likely it would have to be a parser of path through a UI which then populates the various containers / tabs along the way to that content.

Maybe that's overly complex but I'm happy that routing is not a widget/container concern so there is no name conflict.

Jacalz

A very useful addition. I agree with the comments from Drew but I also added some tips/suggestions inline.

Jacalz · 2025-02-05T11:28:08Z

container/navigation.go

+	nav.button.OnTapped = func() {
+		nav.Pop()
+	}


Suggested change

nav.button.OnTapped = func() {

nav.Pop()

}

nav.button.OnTapped = nav.Pop

Different function signature, forgot to explicitly ignore the return value. Still raising the API question, should Pop() return the object?

Jacalz · 2025-02-05T11:30:15Z

container/navigation.go

+//
+// Since: 2.6
+func (nav *Navigation) Push(obj fyne.CanvasObject) {
+	nav.PushWithTitle(obj, nav.titles[len(nav.titles)-1])


I think this will crash if there are 0 titles? Or can that not happen?

Can't happen, but it actually points at something related I was wondering: can the stack be empty or is there always at least one element? Or should it be configurable?

Jacalz · 2025-02-14T08:30:06Z

FYI: This likely closes #1166.

… elements, and adjust tests

coveralls · 2025-02-15T13:23:07Z

coverage: 62.344% (+0.03%) from 62.317%
when pulling b76499b on sdassow:feature/navigation-container
into 1a782f3 on fyne-io:develop.

andydotxyz · 2025-02-25T13:26:36Z

What is the status of this? We have just over 2 days to get any new API in for 2.6.

I'm happy with this change but given the history of the feature I don't feel it would be appropriate for me to accept it.

Jacalz · 2025-02-25T17:42:50Z

The comment by Drew about it only being possible to use this when created using the constructor is still a valid one in my opinion and it hasn't been resolved.

dweymouth · 2025-02-25T19:08:40Z

The comment by Drew about it only being possible to use this when created using the constructor is still a valid one in my opinion and it hasn't been resolved.

Agree - though I think Navigation.stack may be the last one left that needs work for its zero value to be valid/not crash

andydotxyz · 2025-02-25T21:08:54Z

Thanks, sounds like a good summary - @sdassow is this something you can resolve before Friday?

Jacalz · 2025-02-25T21:26:20Z

If it helps, my implementation at https://github.com/Jacalz/rymdport/blob/rewrite/internal/ui/components/stack.go does not have that issue. Feel free to use it as inspiration if it helps.

Jacalz

I'm so sorry for the late review. I really like the approach of the global title and the root object being handled like they are now; it is a big improvement over my navigation container. The support for moving forward to an object that already is added is nice. I think we also want a method to go back without popping?

However, I am a bit sceptical to there being a lot of complexity and manual setup required for a developer to use this (especially when using the struct). It does not quite work like how many of our other APIs work in this regard; it feels to me like internal details are exposed as exported fields in the main struct. I left a few comments inline to try and explain more thoroughly what I mean. I propose handling our internal widgets inside the renderer (by not using a simple renderer), slimming down the custom navigation objects into creating our own buttons internally and letting the developer set OnNext and OnBack functions if they want a different behaviour than default.

Jacalz · 2025-02-28T19:58:29Z

container/navigation.go

+	Title string
+	Back  NavigationObject
+	Next  NavigationObject
+	Label *widget.Label


This seems quite inconsistent with other APIs we have and "Label" is not a descriptive name for where this label ends up. Can we not use a string field for what this text is intended to do and let the internals be managed in our renderer?

Yeah, string is what I would expect here too

Jacalz · 2025-02-28T20:02:04Z

container/navigation.go

+// NavigationObject allows using any object implementing the `fyne.Disableable`,
+// `fyne.CanvasObject`, and `fyne.Tappable` interfaces to be used as Back/Next
+// navigation control.
+//
+// Since: 2.6
+type NavigationObject interface {
+	fyne.CanvasObject
+	fyne.Disableable
+	fyne.Tappable
+}


Like indicated above, I'm sceptical about this interface and the custom solution for changing the backwards and next buttons. I'd prefer to just have an OnBack and an OnNext function to keep things simple. I'd rather begin without this and we can add this later if there is enough demand.

This is a good point - but is slightly ironic because the "forward nagivation" was added in response to it not being sufficient to be called Navigation without it - so we might need a little discussion to agree the scope.

Actually maybe I misunderstood - with or without forward navigation this interface should not be needed.

I didn't really like it either - now gone.

Jacalz · 2025-02-28T20:05:17Z

container/navigation.go

+	nav := &Navigation{
+		Root:  root,
+		Title: s,
+		Label: widget.NewLabelWithStyle(s, fyne.TextAlignCenter, fyne.TextStyle{Bold: true}),
+	}
+	nav.setup()
+	nav.Back = widget.NewButtonWithIcon("", theme.NavigateBackIcon(), func() { _ = nav.Pop() })
+	nav.Back.Disable()
+	nav.Next = widget.NewButtonWithIcon("", theme.NavigateNextIcon(), func() { _ = nav.Forward() })
+	nav.Next.Disable()


This doesn't quite solve the review comment about making sure that the type should be usable from the struct. All fields or exported and part of the struct so sure, you can use the struct but there is a lot of setup required. Have a look at how https://github.com/Jacalz/rymdport/blob/rewrite/internal/ui/components/stack.go has all of the widgets internal in the renderer and handles the setup of them in the CreateRenderer call. That is more consistent with out other APIs and avoid the developer having to do a lot of setup to get the API to work.

Basically, let the renderer contain the internal widgets and do most of the setup in the CreateRenderer call. If the data inside the Navigation struct changes after a refresh, the methods on our custom renderer change the contents of our internal widgets to reflect the change.

Adopted this approach, thanks!

Jacalz · 2025-02-28T20:06:28Z

container/navigation.go

+	if nav.Label != nil {
+		control.Objects = append(control.Objects, nav.Label)
+	}
+	return widget.NewSimpleRenderer(NewBorder(control, nil, nil, nil, &nav.stack))


With the comments above, I think we basically can't use the simple renderer. There needs to be an actual custom renderer with the internal widgets and properties reachable so we can have the methods on that renderer change the internal widgets when values on our widget is changed. You can use https://github.com/Jacalz/rymdport/blob/4707bc88f31670e2de0293f914f0894e30b78755/internal/ui/components/stack.go#L79 for inspiration.

Jacalz · 2025-02-28T20:09:53Z

container/navigation.go

+	Back  NavigationObject
+	Next  NavigationObject


I think this likely adds more complexity over having the CreateRenderer just create buttons internally and let the developer wire up functions for what should happen on next and back. Just doing OnBack func() and OnNext func() will likely suffice. There are two options for that approach:

Always show backwards and forwards buttons and use a sensible approach for selecting what they do when no functions are set (just moving backwards and forwards).

Don't show buttons at all when functions are not set.

Jacalz · 2025-02-28T20:14:33Z

container/navigation.go

+	Label *widget.Label
+
+	level  int
+	stack  fyne.Container


While I do like this approach, it might make more sense (and be more consistent) with just a []fyne.CanvasObject slice and have the container handled in the renderer. See comment further down.

Jacalz · 2025-02-28T20:24:06Z

container/navigation.go

+	return objs[nav.level]
+}
+
+// Forward shows the next object in the again.


There is a word missing here at the end.

andydotxyz · 2025-03-01T18:11:08Z

container/navigation.go

+
+	Root  fyne.CanvasObject
+	Title string
+	Back  NavigationObject


I'm not a fan of exporting these objects either - a standard back and forward buttons should be sufficient

andydotxyz · 2025-03-01T18:16:15Z

container/navigation.go

+// Forward shows the next object in the again.
+//
+// Since: 2.6
+func (nav *Navigation) Forward() fyne.CanvasObject {


I guess we could do with a matching Back as well?

Perhaps indicating in Push/Pop being about manipulating history and Back/Forward about traversing it?

…nderer, adjust tests, and add missing word in documentation

Initial implementation of navigation container

5dd1ba3

Jacalz self-requested a review February 5, 2025 08:31

dweymouth reviewed Feb 5, 2025

View reviewed changes

Jacalz requested changes Feb 5, 2025

View reviewed changes

sdassow added 3 commits February 15, 2025 14:07

Add 'Next' control, make some properties public, allow custom control…

a1fc7fb

… elements, and adjust tests

Merge branch 'develop' into feature/navigation-container

cb8aa71

Fix description

f0c17c4

sdassow added 2 commits February 26, 2025 17:27

Make navigation container support direct struct initialization

1412fab

Merge branch 'develop' into feature/navigation-container

d2b34ed

Jacalz requested changes Feb 28, 2025

View reviewed changes

andydotxyz reviewed Mar 1, 2025

View reviewed changes

sdassow added 4 commits March 8, 2025 13:13

Merge branch 'develop' into feature/navigation-container

5611fc1

Refactor for simpler API, moving some of the logic into a specific re…

a960ca0

…nderer, adjust tests, and add missing word in documentation

Fix json

dc952ab

Remove unused stack container from renderer

b76499b

sdassow requested a review from Jacalz March 8, 2025 16:23

Initial implementation of navigation container #5500

Are you sure you want to change the base?

Initial implementation of navigation container #5500

Conversation

sdassow commented Feb 4, 2025 • edited Loading

Description:

Checklist:

Where applicable:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz commented Feb 14, 2025

coveralls commented Feb 15, 2025 • edited Loading

andydotxyz commented Feb 25, 2025

Jacalz commented Feb 25, 2025

dweymouth commented Feb 25, 2025

andydotxyz commented Feb 25, 2025

Jacalz commented Feb 25, 2025

Jacalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdassow commented Feb 4, 2025 •

edited

Loading

coveralls commented Feb 15, 2025 •

edited

Loading