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

Allow ScrollContainer to be clicked for focus and border #59362

Closed

Conversation

ator-dev
Copy link
Contributor

@ator-dev ator-dev commented Mar 20, 2022

The SceneTree and FileSystem docks have focus borders automatically since TreeDialog is one of the few classes which provides this, and also offer focus-on-click. In my opinion the style of this works well with Godot and is potentially useful for checking what the user is currently doing - for example, if they are editing the SceneTree then that dock has focus.

In order to make the Inspector dock more consistent with this style, since it has a similar feel and context anyway, I have made it (a) clickable and (b) draw the same border when focused. This is a really useful feature for another PR I'm making soon which needs to recognise what dock the user is working in, but it certainly has other use cases (e.g. for plugins) as well and just looks nice.

Problems:

  1. The border-focus stylebox is drawn underneath elements such as text boxes. I don't know if this matters, but it's something that could be fixed if necessary. This has now been fixed by using a more general and standard method for drawing the focus stylebox.
    image
  2. When doing something like dragging a slider immediately after something else was focused, focus returns to that area and not the Inspector. This probably doesn't matter though and, again, could be fixed. This still occurs but is no longer a problem, since recent updates have made focus changes more stable.

Update 2022-09-01: additionally, the focus border has been made available in ScrollContainer as a whole.

@ator-dev
Copy link
Contributor Author

Fullscreen image for reference:
image

@Calinou Calinou added this to the 4.0 milestone Mar 20, 2022
@ator-dev ator-dev force-pushed the enhance-inspector-focus branch 2 times, most recently from dce8b7a to 085682c Compare March 21, 2022 11:27
@ator-dev ator-dev requested a review from a team as a code owner March 21, 2022 11:27
@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Aug 23, 2022
@YuriSizov
Copy link
Contributor

We discussed this in a PR meeting. This is probably good, needs an approving review.

@ator-dev ator-dev force-pushed the enhance-inspector-focus branch from 085682c to 2ba3a4f Compare August 31, 2022 23:13
@ator-dev ator-dev requested review from a team as code owners September 1, 2022 10:24
@ator-dev ator-dev closed this Sep 1, 2022
@ator-dev ator-dev force-pushed the enhance-inspector-focus branch from aa17944 to 0c221f0 Compare September 1, 2022 10:33
@ator-dev ator-dev reopened this Sep 1, 2022
@YeldhamDev YeldhamDev removed request for a team September 1, 2022 13:39
@YeldhamDev YeldhamDev removed request for a team September 1, 2022 13:39
@ator-dev ator-dev force-pushed the enhance-inspector-focus branch from a222882 to 06a016b Compare September 1, 2022 13:47
@ator-dev
Copy link
Contributor Author

ator-dev commented Sep 1, 2022

This has now been generalised in order to make the focus border available in any instance of ScrollContainer. I hope this is OK as I didn't make a proposal about it, but since this behaviour is consistent with Tree and ItemList (using the focus_bg theme property) I think it's a reasonable expansion that should be useful to Godot users and developers.

As a consequence, many more editor docks/panels now have focus borders which makes the editor more consistent and accessible.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 28, 2022

Found a bug in TileSet editor:
uu33KIz7qD
Other than that it looks good.

Here's a rebased patch:
0001-scroll.patch.txt

@ator-dev ator-dev force-pushed the enhance-inspector-focus branch from 06a016b to aa5e785 Compare September 29, 2022 12:08
@ator-dev ator-dev changed the title Allow Inspector to be clicked for focus and border Allow ScrollContainer to be clicked for focus and border Sep 29, 2022
@ator-dev ator-dev force-pushed the enhance-inspector-focus branch from aa5e785 to a9ba52d Compare September 29, 2022 21:08
@KoBeWi
Copy link
Member

KoBeWi commented Sep 29, 2022

Something went wrong and I became author of the commit.

@ator-dev
Copy link
Contributor Author

That's weird, I suppose it was the way I applied the patch. Does it matter?

@ator-dev
Copy link
Contributor Author

By the way, I have addressed your issue with the latest push. I thought that part of the TileSet editor was inconsistent with the rest of Godot anyway, and it could be unclear to users what the panel is if they scroll the header out of view. As you can see I have turned it into a proper scrollable sub-panel (separate case for each TileSet tool):

image

@KoBeWi
Copy link
Member

KoBeWi commented Sep 29, 2022

I think the problem is that you are ignoring clipping for the focus style. Even if you fix tile editor, this makes a generic problem with nested scroll containers and there might be other broken places.

I suppose it was the way I applied the patch. Does it matter?

Probably. There is a way to change the author though. In worst case you can manually modify the patch.

@ator-dev
Copy link
Contributor Author

ator-dev commented Sep 29, 2022

this makes a generic problem with nested scroll containers

That's true - as far as I can tell it happens with Tree too, the focus behaviour of which I was replicating, but I haven't tested fully yet because I find Godot's UI nodes hard to use. I'm not sure yet if it can be easily fixed by using a more appropriate z-index.

@ator-dev ator-dev force-pushed the enhance-inspector-focus branch 2 times, most recently from 76820fc to 2a90293 Compare September 29, 2022 22:00
@ator-dev
Copy link
Contributor Author

I just realised what you meant about ignoring clipping - yes, this is a problem and this solution is more of a hack than a proper fix. I'm not sure how else to get the focus border to show up.

image

@ator-dev ator-dev force-pushed the enhance-inspector-focus branch from 2a90293 to 4b9c8c2 Compare September 30, 2022 16:25
@ator-dev
Copy link
Contributor Author

New state of the TileSet editor, with changes to improve the scrolling behaviour:

tileset-changes

Please let me know if this is big enough to be separated into its own PR.

@ator-dev
Copy link
Contributor Author

The default theme no longer includes borders for ScrollContainers so there should not be a general problem with the slightly hacky method I used.

@pafuent
Copy link
Contributor

pafuent commented Sep 23, 2024

@ator-dev If it's fine with you, I can try to implement this on master
Of course I'll add you as Co-Author, because most of the code will be yours

@ator-dev
Copy link
Contributor Author

Of course, I'd be delighted to see this merged! Go ahead @pafuent :)

@akien-mga
Copy link
Member

Superseded by #97521.

@akien-mga akien-mga closed this Nov 28, 2024
@akien-mga akien-mga removed this from the 4.x milestone Nov 28, 2024
@ator-dev ator-dev deleted the enhance-inspector-focus branch November 28, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants