Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

app,io/system: [API] add StageInactive when window is not in focus #92

Closed
wants to merge 1 commit into from

Conversation

inkeliz
Copy link
Contributor

@inkeliz inkeliz commented Jul 25, 2022

Now, Gio will notify when window focus changes.

Signed-off-by: Inkeliz inkeliz@inkeliz.com

@inkeliz
Copy link
Contributor Author

inkeliz commented Jul 25, 2022

Currently, the behavior of "focus" is defined by the OS, which doesn't match very well. On Windows, the "focus" is lost when some children is focused, on MacOS it's not, but there's another event for that case. However, the FocusEvent can be improved once Gio have better support for children windows.

app/os_x11.go Outdated
@@ -619,8 +619,10 @@ func (h *x11EventHandler) handleEvents() bool {
redraw = (*C.XExposeEvent)(unsafe.Pointer(xev)).count == 0
case C.FocusIn:
w.w.Event(key.FocusEvent{Focus: true})
w.w.Event(system.FocusEvent{Focus: true})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere you send a system.FocusEvent when a key.FocusEvent is also sent. Why is key.FocusEvent not enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key.FocusEvent is going to be consumed by the foremost key handler, which may be deeply nested within application layout code. Wanting to change a window behavior based on the window focus state is a much higher-level concern. One user asked for this focus detection feature in order to build a dmenu-esque application launcher. If the window loses focus, they want to close the window. Doing that by interrogating the state of every widget to see if any of them are focused doesn't seem like the right design.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use key.FocusEvent because of the reason mentioned by @whereswaldon. Also, the FocusEvent can work differently from key.FocusEvent.

I force-push to make the behavior differently. Now, the FocusEvent is only triggered when the window loses focus, instead of the keyboard focus. The word "focus" could be changed to "active", or something else.

Comment on lines 284 to 285
case windows.WM_NCACTIVATE:
w.w.Event(system.FocusEvent{Focus: wParam == windows.TRUE})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows, the KILLFOCUS happens when another window have keyboard focus, which might also happens when some child window is in focus. The NCACTIVATE sounds better, since it report when the window decoration should update.

@eliasnaur
Copy link
Contributor

Currently, the behavior of "focus" is defined by the OS, which doesn't match very well. On Windows, the "focus" is lost when some children is focused, on MacOS it's not, but there's another event for that case. However, the FocusEvent can be improved once Gio have better support for children windows.

This difference between macOS and WIndows is fixed by #93, right?

I don't use key.FocusEvent because of the reason mentioned by @whereswaldon. Also, the FocusEvent can work differently from key.FocusEvent.

I force-push to make the behavior differently. Now, the FocusEvent is only triggered when the window loses focus, instead of the keyboard focus. The word "focus" could be changed to "active", or something else.

Thanks for explaining. I agree that it would be useful to know the active state of an app.Window, separate from the focus. However, I'm not sure a separate event is necessary. Why not expand io/system.Stage with a StageInactive and rename StageRunning to StageActive?

@inkeliz
Copy link
Contributor Author

inkeliz commented Aug 4, 2022

I initially thought about adding a new Stage, but on https://lists.sr.ht/~eliasnaur/gio/%3C96373f04-28fe-6848-25f4-2d9936aa0522%40gmail.com%3E#%3CCAMAFT9VY22-baCO+OyzM+cGH7ktVU08RvgKQOTrbAtAO91beWg@mail.gmail.com%3E, you mention to add that as a new FocusEventt.

However, I'm not sure how the new Stage will work, because we have some checks: if stage < system.StageRunning:

gio/app/window.go

Lines 828 to 835 in b67bef3

if e2.Stage < system.StageRunning {
if w.gpu != nil {
w.ctx.Lock()
w.gpu.Release()
w.gpu = nil
w.ctx.Unlock()
}
}


Currently, the Stage is either "Running" or "Inactive":

gio/io/system/system.go

Lines 61 to 67 in b5f12c5

const (
// StagePaused is the Stage for inactive Windows.
// Inactive Windows don't receive FrameEvents.
StagePaused Stage = iota
// StateRunning is for active Windows.
StageRunning
)

But, in the case of "not in focus" it's both of them, at same time. It's inactive at the perspective of the user, but it's active because it can generate new frames (it still displayed on the screen).

I don't think that it makes sense but maybe we can use 1 << iota +1 and then use StageRunning | StageInactive? So, it's running (gio is generating frames), but it's inactive (the window is not in focus).

@eliasnaur
Copy link
Contributor

I initially thought about adding a new Stage, but on https://lists.sr.ht/~eliasnaur/gio/%3C96373f04-28fe-6848-25f4-2d9936aa0522%40gmail.com%3E#%3CCAMAFT9VY22-baCO+OyzM+cGH7ktVU08RvgKQOTrbAtAO91beWg@mail.gmail.com%3E, you mention to add that as a new FocusEventt.

FocusEvent could work if we can't cram the focused/active state into Stage.

However, I'm not sure how the new Stage will work, because we have some checks: if stage < system.StageRunning:

gio/app/window.go

Lines 828 to 835 in b67bef3

if e2.Stage < system.StageRunning {
if w.gpu != nil {
w.ctx.Lock()
w.gpu.Release()
w.gpu = nil
w.ctx.Unlock()
}
}

This check (and similar) should change to e2.Stage == system.StagePaused.

Currently, the Stage is either "Running" or "Inactive":

gio/io/system/system.go

Lines 61 to 67 in b5f12c5

const (
// StagePaused is the Stage for inactive Windows.
// Inactive Windows don't receive FrameEvents.
StagePaused Stage = iota
// StateRunning is for active Windows.
StageRunning
)

But, in the case of "not in focus" it's both of them, at same time. It's inactive at the perspective of the user, but it's active because it can generate new frames (it still displayed on the screen).

I don't think that it makes sense but maybe we can use 1 << iota +1 and then use StageRunning | StageInactive? So, it's running (gio is generating frames), but it's inactive (the window is not in focus).

Stage was designed to be an integer of increasing "liveness" of the window. So for active, inactive:

 const ( 
 	// StagePaused is the stage for windows that have no on-screen representation. 
 	// Paused Windows don't receive FrameEvents. 
 	StagePaused Stage = iota
        // StageInactive is the stage for windows that are visible, but not active.
        StageInactive
        // Inactive windows receive FrameEvents.
 	// StateRunning is for active and visible Windows.
        // Active windows receive FrameEvents. 
 	StageRunning 
 ) 

io/system/system.go Outdated Show resolved Hide resolved
io/system/system.go Show resolved Hide resolved
io/system/system.go Show resolved Hide resolved
@inkeliz inkeliz force-pushed the focusevent branch 4 times, most recently from 9315868 to dab39c8 Compare September 2, 2022 15:42
@inkeliz inkeliz changed the title app,io/system: add FocusEvent app,io/system: [API] add StageInactive when window is not in focus Sep 2, 2022
app/window.go Outdated Show resolved Hide resolved
app/window.go Outdated Show resolved Hide resolved
app/os_android.go Outdated Show resolved Hide resolved
app/os_android.go Outdated Show resolved Hide resolved
@inkeliz inkeliz force-pushed the focusevent branch 4 times, most recently from 726f626 to 5109d18 Compare September 16, 2022 18:23
Now, Gio will send one system.StageEvent with system.StageInactive when
the window is not active. That is currently implemented to MacOS
and Windows.

This change is not fully backward compatible, if your code compares
the Stage (`stage < system.StageRunning`), you need to consider
the new system.StageInactive.

Signed-off-by: Inkeliz <inkeliz@inkeliz.com>
Signed-off-by: inkeliz <inkeliz@inkeliz.com>
@inkeliz
Copy link
Contributor Author

inkeliz commented Sep 19, 2022

Sorry for a lot of force pushes, but there's no change between them. That fixed comparison, as you suggest.

@eliasnaur
Copy link
Contributor

Merged, thanks!

@eliasnaur eliasnaur closed this Sep 19, 2022
@inkeliz inkeliz deleted the focusevent branch February 24, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants