Decouple the Clipboard API from fyne.Window #5121

pierrec · 2024-09-08T08:20:36Z

Description:

This PR implements the changes for #4418 by:

updating the fyne.App, fyne.Driver and fyne.Window interfaces
migrating tests and cmd programs using the clipboard to the new interface

Fixes #4418

Checklist:

Tests included.
I dont see any automated tests for this but maybe I missed them. I did test manually and ctrl-c, ctrl-v and ctrl-x work as expected on the desktop.
Lint and formatter run with no errors.
Tests all pass.
I get quite a few errors on my system, some related to the locale (labels are emitted in French while tests expect English), others to something else which I dont know about.
They all seem unrelated to this change though.

Where applicable:

Public APIs match existing style and have Since: line.
Any breaking changes have a deprecation path or have been discussed.
Check for binary size increases when importing new modules.
(no new module imported)

Jacalz

Nice work. I changed the base branch to develop and that made the changes look a lot more sensible. Just added a small comment about that we perhaps should consider changing a function signature (if you agree with me there) :)

cmd/fyne_demo/main.go

driver.go

dweymouth · 2024-09-09T15:11:18Z

internal/driver/glfw/window.go

@@ -237,7 +237,7 @@ func (w *window) ShowAndRun() {

 // Clipboard returns the system clipboard
 func (w *window) Clipboard() fyne.Clipboard {
-	return &clipboard{}


Rather than allocating a new clipboard, should we just return the singleton instance from the app here?

Jacalz · 2024-09-09T17:38:54Z

@pierrec Please don't force push your changes very time. Descriptive commits with are lot better as we more easily can see what has changed and GitHub also has a tendency to sometimes lose track of old review comments. We'll squash on merge if needed.

pierrec · 2024-09-09T17:49:15Z

@pierrec Please don't force push your changes very time. Descriptive commits with are lot better as we more easily can see what has changed and GitHub also has a tendency to sometimes lose track of old review comments. We'll squash on merge if needed.

OK, so sorry, will just push next time.

pierrec · 2024-09-09T19:47:48Z

Question while working through the CI failures (a lot of them due to my lack of knowledge of the Fyne codebase and all the build flags!):
what to do with the TestWindow_Clipboard and TestWindow_ClipboardCopy_DisabledEntry tests in internal/driver/glfw/window_test.go? Should they be moved to app/app_test.go?

internal/driver/mobile/driver.go

internal/driver/glfw/driver.go

app/app_test.go

andydotxyz · 2024-09-13T16:33:51Z

I think there may have been a confusion about word usage here - so I want to ensure we are on the same page.

Rather than allocating a new clipboard, should we just return the singleton instance from the app here?

I think that @dweymouth was suggesting that the clipboard exist inside the app instance instead of inside the window where it was before. That is neither a true singleton, but exists only once per application. It will be much more efficient than the current implementation which seems to allocate a new clipboard for every situation where it may be used.

Jacalz · 2024-09-13T23:07:44Z

It will be much more efficient than the current implementation which seems to allocate a new clipboard for every situation where it may be used.

I'm really not sure that it is a correct assumption. All of our clipboard structs are zero size as I can remember and don't actually contain any information so allocating extra size for the interface is unnecessary? We can probably just avoid returning a pointer to the struct and help it not allocate that way.

I don't think it is worth making the app struct larger just for an arbitrary optimisation that we don't if it is worth it or not. Getting the clipboard is likely not a hot path in any application and if anyone cares, they can just save it to a variable themselves and be happy there.

coveralls · 2024-09-14T17:54:18Z

coverage: 66.037% (-0.02%) from 66.058%
when pulling a26d502 on pierrec:issue4418
into 6ea393e on fyne-io:develop.

Jacalz · 2024-09-16T19:56:42Z

Sorry for the long turnaround time for me reviewing this. I've had quite a busy first weeks with the first weeks of my masters program. I'll try to have a look one of the coming days.

pierrec · 2024-09-17T17:04:49Z

Sorry for the long turnaround time for me reviewing this. I've had quite a busy first weeks with the first weeks of my masters program. I'll try to have a look one of the coming days.

Thank you for the update, there is no rush :).

Jacalz

Sorry for the very long wait. Just one minor change needed and we're golden :)

Jacalz · 2024-09-25T08:58:42Z

internal/driver/mobile/driver.go

@@ -100,6 +100,10 @@ func (d *driver) currentWindow() *window {
 	return last
 }

+func (d *driver) Clipboard() fyne.Clipboard {
+	return &mobileClipboard{}


I think this should return a value and not a pointer for consistency? Maybe even use the NewClipboard() function?

This one slipped through. Will do.

… instance

Jacalz · 2024-09-25T10:39:14Z

The static analysis test failure seemed a bit strange. Kicked it to see if it is temporary.

pierrec · 2024-09-25T11:21:16Z

The static analysis test failure seemed a bit strange. Kicked it to see if it is temporary.

I had a look but I dont even have that statement on line 406 of the file; grepping doesnt return any instance of it either.

Ad running it locally returns errors but not that one:

app/icon_cache_file.go:11:5: var once is unused (U1000)
app/icon_cache_file.go:13:19: func (*fyneApp).cachedIconPath is unused (U1000)
app/icon_cache_file.go:30:6: func rootCacheDir is unused (U1000)
app/icon_cache_file.go:35:19: func (*fyneApp).saveIconToCache is unused (U1000)
cmd/fyne/internal/mobile/binres/binres_test.go:249:10: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
internal/driver/glfw/driver_desktop.go:25:7: const desktopDefaultDoubleTapDelay is unused (U1000)
internal/driver/mobile/app/app.go:164:15: func (*app).registerGLViewportFilter is unused (U1000)
widget/check_group.go:185:5: should omit nil check; len() for []*fyne.io/fyne/v2/widget.Check is defined as zero (S1009)
widget/list.go:172:5: should omit nil check; len() for map[fyne.io/fyne/v2/widget.ListItemID]float32 is defined as zero (S1009)
widget/list.go:371:5: should omit nil check; len() for map[fyne.io/fyne/v2/widget.ListItemID]float32 is defined as zero (S1009)
widget/radio_group.go:159:5: should omit nil check; len() for []fyne.io/fyne/v2.CanvasObject is defined as zero (S1009)

Jacalz · 2024-09-28T11:46:18Z

Hmm, interesting. I've never really seen any flukes before there; Staticcheck is about a solid as it gets in static analysis tools for Go. I'll have a look locally when I get back after a short weekend-holiday here and see what the problem can be :)

pierrec · 2024-09-28T12:19:14Z

Hmm, interesting. I've never really seen any flukes before there; Staticcheck is about a solid as it gets in static analysis tools for Go. I'll have a look locally when I get back after a short weekend-holiday here and see what the problem can be :)

Enjoy your break!

andydotxyz · 2024-09-30T17:16:09Z

Perhaps there was a merge and an out-of-order run. kicked it to try again.

Jacalz · 2024-09-30T19:37:41Z

Same error strangely enough. I've opened #5175 for now to hopefully get the ball rolling again :)

Jacalz changed the base branch from master to develop September 8, 2024 10:34

Jacalz reviewed Sep 8, 2024

View reviewed changes

cmd/fyne_demo/main.go Outdated Show resolved Hide resolved

pierrec force-pushed the issue4418 branch from ebdfb61 to 55c6d11 Compare September 8, 2024 11:43

dweymouth requested changes Sep 8, 2024

View reviewed changes

driver.go Outdated Show resolved Hide resolved

dweymouth reviewed Sep 9, 2024

View reviewed changes

dweymouth changed the title ~~Issue4418~~ Decouple the Clipboard API from fyne.Window Sep 9, 2024

Add fyne.App.Clipboard method, fixes fyne-io#4418

83427a8

pierrec force-pushed the issue4418 branch from f4cbbdd to 83427a8 Compare September 9, 2024 15:34

pierre added 2 commits September 10, 2024 08:40

add missing Clipboard for various platforms; update tests

2999a08

test: return proper Clipboard

dd612d7

dweymouth reviewed Sep 11, 2024

View reviewed changes

internal/driver/mobile/driver.go Outdated Show resolved Hide resolved

internal/driver/glfw/driver.go Outdated Show resolved Hide resolved

pierre added 3 commits September 12, 2024 18:44

Clipboard: remove singleton

3482cae

app: add missing NewClipboard()

992f113

app: instantiate a window to initialize glfw for clipboard tests

616fff2

Jacalz reviewed Sep 12, 2024

View reviewed changes

app/app_test.go Outdated Show resolved Hide resolved

app: Clipboard: use test.NewTempApp and NewTempWindow

d6a142f

pierre added 2 commits September 14, 2024 10:15

app: remove duplicat import decl

395444b

Clipboard: do not return pointer for a clipboard instance

a26d502

Jacalz requested changes Sep 25, 2024

View reviewed changes

internal/driver/mobile: use NewClipboard() instead of returning a new…

6cdb472

… instance

Jacalz mentioned this pull request Sep 30, 2024

Use Staticcheck v0.5.1 in CI infrastructure #5175

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple the Clipboard API from fyne.Window #5121

Decouple the Clipboard API from fyne.Window #5121

pierrec commented Sep 8, 2024

Jacalz left a comment

dweymouth Sep 9, 2024

Jacalz commented Sep 9, 2024

pierrec commented Sep 9, 2024

pierrec commented Sep 9, 2024

andydotxyz commented Sep 13, 2024

Jacalz commented Sep 13, 2024

coveralls commented Sep 14, 2024

Jacalz commented Sep 16, 2024

pierrec commented Sep 17, 2024

Jacalz left a comment

Jacalz Sep 25, 2024

pierrec Sep 25, 2024

Jacalz commented Sep 25, 2024

pierrec commented Sep 25, 2024 •

edited

Loading

Jacalz commented Sep 28, 2024

pierrec commented Sep 28, 2024

andydotxyz commented Sep 30, 2024

Jacalz commented Sep 30, 2024

Decouple the Clipboard API from fyne.Window #5121

Are you sure you want to change the base?

Decouple the Clipboard API from fyne.Window #5121

Conversation

pierrec commented Sep 8, 2024

Description:

Checklist:

Where applicable:

Jacalz left a comment

Choose a reason for hiding this comment

dweymouth Sep 9, 2024

Choose a reason for hiding this comment

Jacalz commented Sep 9, 2024

pierrec commented Sep 9, 2024

pierrec commented Sep 9, 2024

andydotxyz commented Sep 13, 2024

Jacalz commented Sep 13, 2024

coveralls commented Sep 14, 2024

Jacalz commented Sep 16, 2024

pierrec commented Sep 17, 2024

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz Sep 25, 2024

Choose a reason for hiding this comment

pierrec Sep 25, 2024

Choose a reason for hiding this comment

Jacalz commented Sep 25, 2024

pierrec commented Sep 25, 2024 • edited Loading

Jacalz commented Sep 28, 2024

pierrec commented Sep 28, 2024

andydotxyz commented Sep 30, 2024

Jacalz commented Sep 30, 2024

pierrec commented Sep 25, 2024 •

edited

Loading