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

Added SetFocus method to the Window #1952

Merged
merged 3 commits into from
Apr 26, 2024
Merged

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
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
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
5 checks passed
@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.

None yet

6 participants