Skip to content

Conversation

@Arugin
Copy link

@Arugin Arugin commented Feb 19, 2024

Summary of the PR

Added SetFocus method to the Window

Related issues, Discord discussions, or proposals

As was discussed here #1526, both GLFW and SDL support this functionality and it is very usefull for the ImGui support, so it can be added to the window.

Further Comments

Unfortunately I wasn't be able to build the project, to check if everything is working correctly, could someone to pick it up? (I am asked for some reason to enter credentials for https://chrome-internal.googlesource.com that I don't have while recursive submodule update. Also I have tons of errors on nuke compile like Restore: \Silk.NET\src\Windowing\Silk.NET.Windowing.Sdl\S
ilk.NET.Windowing.Sdl.csproj : error NU1301: Failed to load service index for source https://gitlab.com/api/v4/projects/47661606/packages/nuget/index.js
on. Silk.NET\Silk.NET.gen.sln)

@Arugin Arugin requested a review from a team as a code owner February 19, 2024 21:23
@Arugin
Copy link
Author

Arugin commented Feb 19, 2024

@dotnet-policy-service agree

@Perksey
Copy link
Member

Perksey commented Feb 19, 2024

This has been added to the API Review queue.

@Perksey Perksey added this to the Next Working Group Meeting milestone Feb 19, 2024
@Perksey
Copy link
Member

Perksey commented Feb 20, 2024

Can you add the API to the PublicAPI files please?

  [ERR] Pack: /Users/runner/work/Silk.NET/Silk.NET/src/Windowing/Silk.NET.Windowing.Common/Interfaces/IView.cs(109,14): error RS0016: Symbol 'SetFocus' is not part of the declared public API [/Users/runner/work/Silk.NET/Silk.NET/src/Windowing/Silk.NET.Windowing.Common/Silk.NET.Windowing.Common.csproj::TargetFramework=netstandard2.0]
  [ERR] Pack: /Users/runner/work/Silk.NET/Silk.NET/src/Windowing/Silk.NET.Windowing.Common/Interfaces/IView.cs(109,14): error RS0016: Symbol 'SetFocus' is not part of the declared public API [/Users/runner/work/Silk.NET/Silk.NET/src/Windowing/Silk.NET.Windowing.Common/Silk.NET.Windowing.Common.csproj::TargetFramework=netstandard2.1]

@Arugin
Copy link
Author

Arugin commented Feb 20, 2024

Sure.

@Arugin Arugin force-pushed the feature/set-focus branch from 0de6f7a to 7c252e0 Compare March 13, 2024 09:56
@Arugin Arugin force-pushed the feature/set-focus branch from 7c252e0 to 78a9d21 Compare March 24, 2024 07:24
@Arugin
Copy link
Author

Arugin commented Apr 24, 2024

Any ETA when it can be merged? Or I need to do something else so it would be ready for merge?

@Perksey
Copy link
Member

Perksey commented Apr 24, 2024

We were waiting for another API review session, but given this is just one API it probably isn't worth the wait.

@HurricanKai
Copy link
Member

I haven't tested this, but according to docs SDLs RaiseWindow both requests the input focus (so from my understanding typing would go directly into the window, without any further interaction) and also brings it to the front
GLFW does not document any changing of the window "layer".
SDLs SetWindowInputFocus seems to be more in line with GLFWs (documented) functionality.

EDIT: I learned that although docs are confusing GLFW does in fact also couple both in FocusWindow

It could be considered to rename this to InputFocus and limit this to only that.
EDIT: Given new info I'm fine with this PR as-is.
The below is still useful information, and I wonder how NoBringToFrontOnFocus works on GLFW, but whatever. ImGUI pop-ups are cool, but maybe not the most important thing.

Looking at the original issue this seems to be about ImGUI multi-window.
I've never hooked this up myself, so not sure what the hooks look like, but ImGUI does also make this distinction quite explicitly. There's a difference between being on top and having input focus.
ImGUI controls this using ImGuiWindowFlags_NoBringToFrontOnFocus
This is explicitly useful when implementing pop ups, especially things like Auto-complete where it's important input focus is retained.

My suggestion would be to explicitly split this into BringToFront (naming?) and [Request]InputFocus
Although it's unclear how to implement this in GLFW.
EDIT: As said above, I'm also fine with this PR as-is. The suggestion stands, although it may be impossible.

@Arugin Arugin force-pushed the feature/set-focus branch from 7b5fad6 to bf55028 Compare April 25, 2024 07:28
@Arugin
Copy link
Author

Arugin commented Apr 26, 2024

I've renamed the method according to comments and rebased the branch.

@ThomasMiz ThomasMiz merged commit c6c0e34 into dotnet:main Apr 26, 2024
@ThomasMiz
Copy link
Contributor

👌👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants