window. add close button interceptor to react on the window closing before anything is done #1180

ghost · 2020-07-10T10:17:15Z

Description:

adding wrapper to closing handler so that we can react on the window closing before the window is destroyed.
for example it can serve window closing confirmation

Fixes #467

Checklist:

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

Where applicable:

Public APIs match existing style.
Any breaking changes have a deprecation path or have been discussed.

example:

func main() {
	app := app.New()
	win := app.NewWindow("window title")
	win.Resize(fyne.NewSize(200, 100))

	win.SetCloseIntercept(func() {
		dialog.ShowConfirm("title", "close?",
			func(response bool) {
				if response {
					win.Close()
				}
			}, win)
	})

	win.SetContent(widget.NewHBox(
		widget.NewLabel("long text to exceed initial window width"),
	))

	win.ShowAndRun()
}

andydotxyz

If the idea of ‘WillClose’ is to allow vetoing then perhaps it could be named more accordingly?

The glfw implementation seems complex - does it need a ‘closing’ bool and a goroutine? Isn’t it a case of calling the Function to verify, then continuing if ok? If the code was rearranged to exit early the diff could be much smaller.

Also why is the gomobile driver a no-op? Should we not be able to veto closing windows there?

ghost · 2020-07-12T03:54:44Z

Yes, the main benefit is to get the place to prevent the window closing, but there could be different cases where onClose is too late because window is already closed. I took name from the original discussion and if you see there's better option just write it.

closing yes it should be there, otherwise it's possible to get several handlers running that can create races. For example if you build my example and try to close the window several times without this protection it will show several confirmation dialogs.

Simple function call without goroutine is ok for a plain go code but using ShowConfirm in the handler is taking the app to a deadlock (checked on linux and windows). I'll keep old closed as doClose to simplify the diff

The only reason of no-op is i'm not sure how to check that case. I'll add the code there as well

ghost · 2020-07-12T07:24:12Z

rebased
old close is renamed to doCLose to reuse in willClose and make the diff easier to read
mobile part is implemented, checked on pixel 2 simulator.

There's an issue related to the ShowConfirm call already in the codebase (for mobile).
Calling it from the button click handler for example is not showing the dialog so i was checking my code using new window with two buttons. My bad, set wrong parent window for ShowConfirm, everything is OK

andydotxyz · 2020-07-12T09:16:58Z

Isn’t the race condition that requires ‘closing’ only present because of the goroutine?

If a confirm is required I would
Imagine that showing a confirm then vetoing the close, followed by a programmatic close if the user confirmed is a pretty standard workflow - this should not require goroutines should it?

ghost · 2020-07-12T09:23:17Z

I had no issues with the goroutine present but every time i've tried to remove it i'm experiencing a deadlock.
This could be related to the viewport. Closed is connected to it and looks like these handlers should not block the flow.

The easiest way to see the closing case is the window close [X] button that i'm not sure can be blocked

andydotxyz · 2020-07-12T17:34:51Z

Isn't the deadlock related to the example code which blocks waiting for the interface to do something?

By moving to a goroutine you add the need to check for concurrent runs. Maybe this relates to the design - "WillClose" sounds like a notification or event but really it's supporting, or requiring, a long blocking wait whilst the user is asked of something. This is counter to how other callbacks are setup whereby they use the event thread and blocking would be a bad idea.

andydotxyz · 2020-07-12T17:47:18Z

Thinking out loud a simpler design might require that if WillClose is set then the developer must call Window.Close() if that is what they want? That way threading is not required as the dialog callback can simply call that method if appropriate...

ghost · 2020-07-12T18:58:36Z

willClose is replacing the internal closing procedure if set and developer should call Close() explicitly in the willClose
It's not solving the deadlock and closing need, just making it client code responsibility. And only the external close command is covered by willClose

andydotxyz

Just a few inline comments, it looks cleaner but would like a third opinion

test/testwindow.go

window.go

internal/driver/gomobile/window.go

andydotxyz · 2020-07-12T20:15:27Z

I think it looks cleaner this way - but perhaps others could feed in?
@toaster @stuartmscott @okratitan @lucor

ghost · 2020-07-13T08:14:16Z

closing this PR to reopen and get additional commit after recent guthub issues

ghost · 2020-07-13T08:30:18Z

one of the text selection tests failed on one node, not sure it's related:

?   	fyne.io/fyne/tools/playground	[no test files]
--- FAIL: TestEntry_Select (0.50s)
    --- FAIL: TestEntry_Select/delete_select_down_with_Shift_still_hold (0.11s)
        util.go:60: 
            	Error Trace:	util.go:60
            	            				entry_test.go:1287
            	Error:      	Not equal: 
            	Test:       	TestEntry_Select/delete_select_down_with_Shift_still_hold
            	Messages:   	Image did not match master. Actual image written to file:///home/travis/gopath/src/fyne.io/fyne/widget/testdata/failed/entry/selection_initial.png.
2020/07/13 08:17:46 Fyne error:  Entry does not allow Truncation

andydotxyz · 2020-07-13T08:35:44Z

I have restarted in case it was a temporary fault

internal/driver/glfw/window.go

internal/driver/glfw/window_test.go

ababo · 2020-07-14T10:51:05Z

internal/driver/glfw/window.go

@@ -404,6 +409,13 @@ func (w *window) Close() {
 	w.closed(w.viewport)
 }

+func (w *window) DoClose() {


Do we really need that to be exported?

yes, the idea is that user should explicitly close the window in this new handler

IMO a difference between Close() and DoClose()isn't clear intuitively. I would stick with TryClose() and onCloseTry naming scheme. @andydotxyz What do you think?

DoClose is expected to close window without additional checks so TryClose is exactly opposite name

Yep, that what I meant. Don't these names look more intuitive?

@RustamGamidov Can you do as @andydotxyz suggests, i.e. rename willClose into onCloseIntercept, SetWillClose() into SetCloseIntercept() and get rid of DoClose()? Then the only thing that will remain is an approve from another contributor.

yeah... made it if i understood the idea. I'm just confused with how many things are moved to the client code like checking for deadlocks or loops. Especially because there was a comment regarding possible loop

It should not be neccessary to do any checking for loops or deadlocks in client code. The simplicity we are looking for is simplicity for the developers using the API.
They should not have to worry about threading at all, that is one of our design principles.

then i believe onCloseIntercepted should be called in goroutine like it was initially but it probably worth to wait for comments from other repo owners because we already used our arguments and most likely we'll stuck arguing

I wonder if perhaps the event queue is the right place to put it? Just launching goroutines to work around possible deadlocks seems like it's not fixing the underlying issue? We used to do that with button taps and key presses and it caused a jumble of out-of-order events, so we don't do that any more.

internal/driver/gomobile/window.go

window.go

ghost · 2020-07-15T11:26:21Z

same test failed on the same node as far as i remember that still looks not related

?   	fyne.io/fyne/tools/playground	[no test files]
--- FAIL: TestEntry_Select (0.25s)
    --- FAIL: TestEntry_Select/delete_select_down_with_Shift_still_hold (0.06s)
        util.go:60: 
            	Error Trace:	util.go:60
            	            				entry_test.go:1287
            	Error:      	Not equal: 
            	Test:       	TestEntry_Select/delete_select_down_with_Shift_still_hold
            	Messages:   	Image did not match master. Actual image written to file:///home/travis/gopath/src/fyne.io/fyne/widget/testdata/failed/entry/selection_initial.png.

btw, locally on win10 i'm experiencing not stable clipboard test

andydotxyz

I think the when-to-check-intercept logic is reversed in this latest version, which is probably why you are seeing loops?

internal/driver/glfw/window.go

internal/driver/glfw/window_test.go

internal/driver/gomobile/window.go

test/testwindow.go

okratitan · 2020-07-15T15:04:04Z

So I think my feeling on this is - I like the style of WillClose with a return boolean. The Intercept requiring the dev to call Close() is a little confusing/not obvious in my opinion. However - WillClose is probably the wrong name and the name of that Method will need to be revisited.

On the flip side the implementation will be easier/cleaner with Intercept - as WillClose will have to involve some goroutines etc... So there is a trade off either way. I will be happy going with what the majority decides.

Jacalz

I think that I quite like the intercept behaviour that the PR implements currently. It looks like the example isn't updated though.

…nything is done

window.Close() should be called explicitly to close the window

to keep Close for the programmatic closing with pre-checks to break the pre-checks loop without status variable

be aware client code should close the window, check for loops and ensure there are locks

… window tests are deleted because

ghost · 2020-08-11T05:53:15Z

fixed the example and rebased over the current develop

Jacalz

This looks good to me. Thanks for rebasing and updating the example 👍

andydotxyz

This looks great and is very close :) two comments inline that I think were missed from earlier discussion threads.
Last thought - in previous version there were tests, is it possible to re-add testing to this version?

andydotxyz · 2020-08-11T10:14:41Z

internal/driver/glfw/window.go

-	if w.onClosed != nil {
-		w.queueEvent(w.onClosed)
+	if w.onCloseIntercepted != nil {
+		w.onCloseIntercepted()


Earlier you mentioned that this could block - should it be passed to w.queueEvent instead?

i've tried that, and it's not helping unfortunately
i can do this to follow the pattern

Can you confirm what the issue is please?
Looking at the example above I can't help but feel the channel should not be needed - just calling Window.Close() in the callback should work.

the issue is not reacting ShowConfirm
Yes, it's solved with removing the channel with and without using queueEvent. Changed the example accordingly

andydotxyz · 2020-08-11T10:21:49Z

window.go

@@ -64,6 +64,9 @@ type Window interface {
 	// SetOnClosed sets a function that runs when the window is closed.
 	SetOnClosed(func())

+	// SetCloseIntercept sets a function that runs instead of closing if defined


Can you add a "." to this, and add another line instructing people to call Close() to confirm window closure?

ghost · 2020-08-11T11:40:19Z

is it possible to re-add testing to this version?

i don't see the way how to do this as the current solution is catching window manager closing only that is private

no specific reason for that just following the pattern to decrease deadlock chance

ghost · 2020-08-11T12:02:04Z

# cd .; git clone -- https://github.com/fzipp/gocyclo /Users/travis/gopath/src/github.com/fzipp/gocyclo
Cloning into '/Users/travis/gopath/src/github.com/fzipp/gocyclo'...
fatal: unable to access 'https://github.com/fzipp/gocyclo/': Failed to connect to github.com port 443: Operation timed out
package github.com/fzipp/gocyclo: exit status 128

quite unlikely related to the PR

andydotxyz · 2020-08-11T12:31:14Z

i don't see the way how to do this as the current solution is catching window manager closing only that is private

But from tests you can call private methods...

ghost · 2020-08-11T13:04:16Z

oh indeed! missed they are in the same package
made it in one test that makes tests dependent on the previous code but i see it's the common practice in the project

ghost · 2020-08-11T13:15:34Z

ci failed on one of the nodes

--- FAIL: TestFileWatcher_FileDeleted (0.19s)
    settings_desktop_test.go:92: File watcher callback was not called

andydotxyz

Great thanks so much for getting this the whole way

andydotxyz · 2020-08-12T09:16:14Z

@okratitan I think that @toaster and I are happy here, can you approve the latest version too please?

andydotxyz · 2020-08-14T13:47:43Z

Many thanks, this will be in 1.4 release later this year.

ghost · 2020-08-14T13:52:44Z

🙏 thanks a lot for all your patience and comments! That was the long way!

andydotxyz requested changes Jul 11, 2020

View reviewed changes

andydotxyz requested changes Jul 12, 2020

View reviewed changes

test/testwindow.go Outdated Show resolved Hide resolved

window.go Outdated Show resolved Hide resolved

internal/driver/gomobile/window.go Outdated Show resolved Hide resolved

ghost closed this Jul 13, 2020

ghost reopened this Jul 13, 2020

ababo reviewed Jul 14, 2020

View reviewed changes

andydotxyz requested changes Jul 15, 2020

View reviewed changes

okratitan self-requested a review July 15, 2020 14:54

Jacalz reviewed Aug 10, 2020

View reviewed changes

Rustam Gamidov added 6 commits August 11, 2020 08:40

window. add willClose handler to react on the window closing before a…

75e1ce9

…nything is done

window. implement willClose for mobile

2c0e228

window. willClose to replace internal closing procedure

b492c0e

window.Close() should be called explicitly to close the window

window. add DoClose method closing window without pre-checks

1f15580

to keep Close for the programmatic closing with pre-checks to break the pre-checks loop without status variable

window. rename to SetCloseIntercept and minimize other changes

b9e10a4

be aware client code should close the window, check for loops and ensure there are locks

window. move interception out of Close so Close is simply closing the…

83f2c83

… window tests are deleted because

Jacalz previously approved these changes Aug 11, 2020

View reviewed changes

andydotxyz requested changes Aug 11, 2020

View reviewed changes

window. push close interceptor to the queueEvent instead of direct call.

4e37d25

no specific reason for that just following the pattern to decrease deadlock chance

ghost dismissed Jacalz’s stale review via 4e37d25 August 11, 2020 11:46

ghost changed the title ~~window. add willClose handler to react on the window closing before anything is done~~ window. add close button interceptor to react on the window closing before anything is done Aug 11, 2020

window. test close interception functionality

69be504

window. close interception tests to check onClosed calls

6711148

andydotxyz approved these changes Aug 12, 2020

View reviewed changes

okratitan approved these changes Aug 14, 2020

View reviewed changes

andydotxyz merged commit b210665 into fyne-io:develop Aug 14, 2020

ghost deleted the window-willClose branch August 14, 2020 13:52

window. add close button interceptor to react on the window closing before anything is done #1180

window. add close button interceptor to react on the window closing before anything is done #1180

Conversation

ghost commented Jul 10, 2020 • edited by ghost

Description:

Checklist:

Where applicable:

example:

andydotxyz left a comment

Choose a reason for hiding this comment

ghost commented Jul 12, 2020

ghost commented Jul 12, 2020 • edited by ghost

andydotxyz commented Jul 12, 2020

ghost commented Jul 12, 2020 • edited by ghost

andydotxyz commented Jul 12, 2020 • edited

andydotxyz commented Jul 12, 2020

ghost commented Jul 12, 2020 • edited by ghost

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz commented Jul 12, 2020

ghost commented Jul 13, 2020 • edited by ghost

ghost commented Jul 13, 2020 • edited by ghost

andydotxyz commented Jul 13, 2020

Choose a reason for hiding this comment

ghost Jul 14, 2020 • edited by ghost

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

ghost Jul 15, 2020 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 15, 2020 • edited by ghost

andydotxyz left a comment

Choose a reason for hiding this comment

okratitan commented Jul 15, 2020

Jacalz left a comment

Choose a reason for hiding this comment

ghost commented Aug 11, 2020

Jacalz left a comment

Choose a reason for hiding this comment

andydotxyz 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

ghost Aug 11, 2020 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 11, 2020

ghost commented Aug 11, 2020

andydotxyz commented Aug 11, 2020

ghost commented Aug 11, 2020

ghost commented Aug 11, 2020

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz commented Aug 12, 2020

andydotxyz commented Aug 14, 2020

ghost commented Aug 14, 2020

ghost commented Jul 10, 2020 •

edited by ghost

ghost commented Jul 12, 2020 •

edited by ghost

ghost commented Jul 12, 2020 •

edited by ghost

andydotxyz commented Jul 12, 2020 •

edited

ghost commented Jul 12, 2020 •

edited by ghost

ghost commented Jul 13, 2020 •

edited by ghost

ghost commented Jul 13, 2020 •

edited by ghost

ghost Jul 14, 2020 •

edited by ghost

ghost Jul 15, 2020 •

edited by ghost

ghost commented Jul 15, 2020 •

edited by ghost

ghost Aug 11, 2020 •

edited by ghost