Add the ability to disable ToolbarActions #2306 #4347

dominikhauk · 2023-10-28T11:41:19Z

Description:

Added ability to disable ToolbarActions.
I chose to embed the button directly, since it already handles the disable-state logic.
Also, since the button is returned as a pointer, no separate refreshing logic is required. This seems to have been an issue with previous PRs for this issue.

Please let me know if the "Since:" comment with version 2.5 is fine, or if you prefer another version.

I don't necessarily like that the state of Icon & OnActivated are now essentially duplicated (once in the ToolbarAction and once in the underlying Button itself), but I did not want to break backward compatibility by changing the public interface. Let me know what your thoughts are on this.

Fixes #2306

I have tested the code using the following addition to the fyne-demo (cmd/fyne_demo/tutorials/widget.go):

func makeToolbarTab(_ fyne.Window) fyne.CanvasObject {
	paste := widget.NewToolbarAction(theme.ContentPasteIcon(), func() { fmt.Println("Paste") })

	t := widget.NewToolbar(widget.NewToolbarAction(theme.MailComposeIcon(), func() { fmt.Println("New") }),
		widget.NewToolbarSeparator(),
		widget.NewToolbarSpacer(),
		widget.NewToolbarAction(theme.ConfirmIcon(), func() {
			if paste.Disabled() {
				paste.Enable()
			} else {
				paste.Disable()
			}
		}),
		widget.NewToolbarSpacer(),
		widget.NewToolbarAction(theme.ContentCutIcon(), func() { fmt.Println("Cut") }),
		widget.NewToolbarAction(theme.ContentCopyIcon(), func() { fmt.Println("Copy") }),
		paste,
	)

	return container.NewBorder(t, nil, nil, nil)
}

Checklist:

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

Where applicable:

Public APIs match existing style and have Since: line.

widget/toolbar.go

dominikhauk · 2023-10-29T08:41:29Z

I also experience some weird rendering behavior in the fyne_demo after making my changes. After a refresh, the alignment of the toolbar items seems to be off. I am not sure if this a local bug for me, or if it is caused by my changes:

Before:

After refresh:

The code that I am using is

func makeToolbarTab(_ fyne.Window) fyne.CanvasObject {
	paste := widget.NewToolbarAction(theme.ContentPasteIcon(), func() { fmt.Println("Paste") })

	var t *widget.Toolbar

	t = widget.NewToolbar(widget.NewToolbarAction(theme.MailComposeIcon(), func() { fmt.Println("New") }),
		widget.NewToolbarSeparator(),
		widget.NewToolbarSpacer(),
		widget.NewToolbarAction(theme.ConfirmIcon(), func() {
			if paste.Disabled {
				paste.Disabled = false
			} else {
				paste.Disabled = true
			}
			t.Refresh()
		}),
		widget.NewToolbarSpacer(),
		widget.NewToolbarAction(theme.ContentCutIcon(), func() { fmt.Println("Cut") }),
		widget.NewToolbarAction(theme.ContentCopyIcon(), func() { fmt.Println("Copy") }),
		paste,
	)

	return container.NewBorder(t, nil, nil, nil)
}

Could you try it on your side and let me know if it happens as well on your machine?

andydotxyz · 2023-10-29T11:08:57Z

Wasn't implementing the disablable interface desirable though? Maybe it isn't possible as the toolbar action is not really a widget?

dominikhauk · 2023-10-30T16:48:14Z

Wasn't implementing the disablable interface desirable though? Maybe it isn't possible as the toolbar action is not really a widget?

I intentionally used the Disabled property, since this is also what the MenuItem in menu.go uses, and I wanted to stay consistent here.

I am not sure how you handled such situations in the past. Let me know if I should change the implementation, or how you want to continue.

widget/toolbar.go

dominikhauk · 2023-10-31T07:29:52Z

By the way, what is the reasoning behind not making the toolbarItems widgets?
According to my current understanding, we would not be having these issues if the toolbarItems were normal widgets, right?
For example, a menuItem also is a widget:

fyne/widget/menu_item.go

Line 42 in ccc6a1c

widget.Base

coveralls · 2023-11-07T20:49:44Z

coverage: 64.629% (+0.004%) from 64.625%
when pulling 62610f6 on maruu:2306-disable-toolbar-actions
into 4f617dc on fyne-io:develop.

dominikhauk · 2023-11-08T12:08:08Z

Rebased branch to current develop state

andydotxyz · 2023-11-08T15:55:39Z

By the way, what is the reasoning behind not making the toolbarItems widgets?

According to my current understanding, we would not be having these issues if the toolbarItems were normal widgets, right?

For example, a menuItem also is a widget:

fyne/widget/menu_item.go

Line 42 in ccc6a1c

widget.Base

It's about the public API. Internally menuItem is a widget, but the public api fyne.MenuItem is not. Using public APIs that are abstractions allows us to keep the widgets consistent.

A container will give the user control over its content through adding arbitrary widgets, but it is rare for a widget to do this. If you want a "toolbar that accepts anything" then wouldn't that just be a VBox with a background rectangle?

That said I don't know what the issue is here, the ToolbarObject returns a CanvasObject for its "implementation detail" and this can be a widget, disablable widget or anything else. So why would this be easier if the main object was a widget?

andydotxyz

I can't quite see how the disabled item is refreshed without internal API access.
I try to give more detail inline - let me know if it is not clear.

widget/toolbar.go

widget/toolbar_test.go

dominikhauk · 2023-12-02T16:55:39Z

Any idea why the tests are always failing with:

D:\a\fyne\setup-go-faster\go\1.17.13\x64\pkg\tool\windows_amd64\link.exe: running gcc failed: exit status 1
C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3164744842\000008.o: in function `_cgo_preinit_init':
c:\go\src\runtime\cgo/gcc_libinit_windows.c:30: undefined reference to `__imp___iob_func'
C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3164744842\000008.o: in function `x_cgo_sys_thread_create':
c:\go\src\runtime\cgo/gcc_libinit_windows.c:60: undefined reference to `__imp___iob_func'
C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3164744842\000008.o: in function `x_cgo_notify_runtime_init_done':
c:\go\src\runtime\cgo/gcc_libinit_windows.c:101: undefined reference to `__imp___iob_func'
C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3164744842\000009.o: in function `x_cgo_thread_start':
c:\go\src\runtime\cgo/gcc_util.c:18: undefined reference to `__imp___iob_func'
C:/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\RUNNER~1\AppData\Local\Temp\go-link-3164744842\000010.o: in function `_cgo_sys_thread_start':
c:\go\src\runtime\cgo/gcc_windows_amd64.c:31: undefined reference to `__imp___iob_func'
collect2.exe: error: ld returned 1 exit status

? I don't see any connection to the code that I have changed.

Jacalz · 2023-12-02T22:37:53Z

@maruu It is not your fault. It’s a bug in versions of Go older than 1.20. We need to look at how we should address that on our next contributor meeting for Fyne.

andydotxyz

More thoughts inline - good to have the feature working though.

dominikhauk · 2024-01-16T08:46:46Z

Hi @andydotxyz @dweymouth, as discussed here: #4347 (comment) I changed the PR to now use the Button directly. The benefits from my point of view are that the state management is not duplicated, but can be reused from the Button itself.

I again tested the functionality with an extension of the fyne_demo app (see my initial PR description for the code if you want to give it a try). Let me know what you think.

…ctionButton" This reverts commit 97ecd65.

dominikhauk · 2024-01-16T08:50:16Z

Rebased branch to current develop state

andydotxyz

Looking good, but this will crash if you just create a &ToolbarAction{} directly as the constructor function is optional.

Since the ToolbarAction can also be created without the constructor, the pointer to the button could be nil.

dominikhauk · 2024-01-19T15:04:34Z

Looking good, but this will crash if you just create a &ToolbarAction{} directly as the constructor function is optional.

You are totally correct. Since I cannot store a reference to the button directly (because it cannot be copied) I chose to use lazy initialization. I also added a test for this, hope that's okay. Let me know what you think.

widget/toolbar.go

Jacalz

Thanks for working on this. I have a few suggestions that will clean up the code a bit once resolved

widget/toolbar.go

…lization

Jacalz · 2024-02-01T09:33:23Z

Please resolve the issues reported by the static analysis workflow

andydotxyz

Well done and thanks, this is a great addition

dominikhauk marked this pull request as ready for review October 28, 2023 11:45

dweymouth reviewed Oct 28, 2023

View reviewed changes

widget/toolbar.go Outdated Show resolved Hide resolved

widget/toolbar.go Outdated Show resolved Hide resolved

dominikhauk commented Oct 29, 2023

View reviewed changes

widget/toolbar.go Outdated Show resolved Hide resolved

dweymouth reviewed Oct 30, 2023

View reviewed changes

widget/toolbar.go Outdated Show resolved Hide resolved

dweymouth reviewed Oct 30, 2023

View reviewed changes

widget/toolbar.go Outdated Show resolved Hide resolved

dominikhauk requested a review from dweymouth November 3, 2023 08:24

dominikhauk force-pushed the 2306-disable-toolbar-actions branch from 0634bc7 to 4dc9db5 Compare November 8, 2023 12:07

andydotxyz requested changes Nov 26, 2023

View reviewed changes

widget/toolbar.go Show resolved Hide resolved

widget/toolbar.go Outdated Show resolved Hide resolved

widget/toolbar_test.go Outdated Show resolved Hide resolved

andydotxyz requested changes Dec 30, 2023

View reviewed changes

dominikhauk requested a review from andydotxyz January 16, 2024 08:46

maruu and others added 8 commits January 16, 2024 08:49

add the ability to disable ToolbarActions #2306

873bdcd

test that updating the action function works

4e0173d

add version statement to new public API

8f8a6ab

separate state from renderer by introducing internal toolbarActionButton

7e34b48

extend the base widget for toolbarActionButton

6227784

Revert "separate state from renderer by introducing internal toolbarA…

8b47ba7

…ctionButton" This reverts commit 97ecd65.

GH-2306: remove unnecessary refresh

df55695

GH-2306: sync Icon property

a701d5f

dominikhauk force-pushed the 2306-disable-toolbar-actions branch from 636deb5 to a701d5f Compare January 16, 2024 08:50

andydotxyz requested changes Jan 17, 2024

View reviewed changes

GH-2306: use lazy initialization of button

Loading
Loading status checks…

d9f4e9d

Since the ToolbarAction can also be created without the constructor, the pointer to the button could be nil.

dominikhauk commented Jan 19, 2024

View reviewed changes

widget/toolbar.go Outdated Show resolved Hide resolved

dominikhauk requested a review from andydotxyz January 19, 2024 15:05

Jacalz requested changes Jan 19, 2024

View reviewed changes

widget/toolbar.go Outdated Show resolved Hide resolved

widget/toolbar.go Outdated Show resolved Hide resolved

widget/toolbar.go Outdated Show resolved Hide resolved

dominikhauk added 2 commits January 31, 2024 07:46

GH-2306: save button directly instead of pointer to avoid lazy initia…

d424c9b

…lization

GH-2306: add clarification comment to test

Loading
Loading status checks…

a8975a5

dominikhauk requested a review from Jacalz January 31, 2024 07:47

GH-2306: remove whitespaces

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

Loading
Loading status checks…

62610f6

andydotxyz approved these changes Feb 1, 2024

View reviewed changes

Jacalz approved these changes Feb 1, 2024

View reviewed changes

Jacalz merged commit 34502cd into fyne-io:develop Feb 1, 2024
12 checks passed

dweymouth mentioned this pull request May 8, 2024

Add the ability to disable ToolbarActions #2306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to disable ToolbarActions #2306 #4347

Add the ability to disable ToolbarActions #2306 #4347

dominikhauk commented Oct 28, 2023 •

edited

Loading

dominikhauk commented Oct 29, 2023

andydotxyz commented Oct 29, 2023

dominikhauk commented Oct 30, 2023

dominikhauk commented Oct 31, 2023

coveralls commented Nov 7, 2023 •

edited

Loading

dominikhauk commented Nov 8, 2023

andydotxyz commented Nov 8, 2023

andydotxyz left a comment

dominikhauk commented Dec 2, 2023

Jacalz commented Dec 2, 2023

andydotxyz left a comment

dominikhauk commented Jan 16, 2024

dominikhauk commented Jan 16, 2024

andydotxyz left a comment

dominikhauk commented Jan 19, 2024

Jacalz left a comment

Jacalz commented Feb 1, 2024

andydotxyz left a comment

Add the ability to disable ToolbarActions #2306 #4347

Add the ability to disable ToolbarActions #2306 #4347

Conversation

dominikhauk commented Oct 28, 2023 • edited Loading

Description:

Checklist:

Where applicable:

dominikhauk commented Oct 29, 2023

andydotxyz commented Oct 29, 2023

dominikhauk commented Oct 30, 2023

dominikhauk commented Oct 31, 2023

coveralls commented Nov 7, 2023 • edited Loading

dominikhauk commented Nov 8, 2023

andydotxyz commented Nov 8, 2023

andydotxyz left a comment

Choose a reason for hiding this comment

dominikhauk commented Dec 2, 2023

Jacalz commented Dec 2, 2023

andydotxyz left a comment

Choose a reason for hiding this comment

dominikhauk commented Jan 16, 2024

dominikhauk commented Jan 16, 2024

andydotxyz left a comment

Choose a reason for hiding this comment

dominikhauk commented Jan 19, 2024

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz commented Feb 1, 2024

andydotxyz left a comment

Choose a reason for hiding this comment

dominikhauk commented Oct 28, 2023 •

edited

Loading

coveralls commented Nov 7, 2023 •

edited

Loading