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

Fix hiDPI scaling for dynamic options + empty value triggering suggestions #2077

Merged
merged 4 commits into from
Feb 11, 2024

Conversation

Atlinx
Copy link
Contributor

@Atlinx Atlinx commented Feb 6, 2024

Fixes

Code Changes

  • Resized portrait.svg to 16 x 16
  • Make Suggestions box use a fixed icon size of 16 x 16 (Scaled to your editor size)
  • Calculates suggestion box size using line_height, v_separation, h_separation, icon texture size, and icon_margin instead of using a magic number.
  • Update the height and width on every change
  • Update suggestions box width on the next frame in case PanelContainer changes its width
  • Removes or current_value == "" from the _on_search_focus_entered function, since searching shouldn't depend on current_value
    • Sometimes the value passed into the field is meant to be empty (""), so we don't want to show the suggestions box in that case.
    • Ex. when we want a "(No one)" character_identifier, its actually represented as an empty ("") value.

@Jowan-Spooner
Copy link
Collaborator

Hey @Atlinx thanks again for the PR. I tried giving some feedback on discord, but I'll post it here again.

When testing your HiDPI PR on my (not HiDPI) screen:

  • the suggestions box now sometimes cuts off the suggestions.
    grafik

  • Also as I was trying to do in my PR the minimum width should be the width of the whole field so we don't get a very short suggestions box
    grafik

  • the resizing sometimes does some strange stuff (see video)

2024-02-07-09-09-26.mp4

The video doesn't work on firefox for me idk why, on chrome it works

Also for this

Removes or current_value == "" from the _on_search_focus_entered function, since searching shouldn't depend on current_value

While I'm okay with changing this (to fix the box not closing), it's wasn't really a bug but an intended qol feature. It's because it's very typical for these "empty" values to be the defaults and thus to be changed a lot. This line means you can simply click anywhere in the field to open the suggestions in these special cases where it is very likely you want to change them.
I'm open for removing this though or finding a different solution, just wanted to say it wasn't really a mistake.

- Use fixed icon size for suggestions box
- Resized portrait.svg to 16x16 (Was 22 x 22 originally)
@Atlinx
Copy link
Contributor Author

Atlinx commented Feb 9, 2024

Thanks for testing my PR! I've fixed the clipping in the latest commit by accounting for items that use an editor_icon instead of an icon. I've also made the suggestions box have a fixed icon size to ensure consistency, and I've resized the portrait.svg to a 16x16 image (Was 22 x 22 originally). Please let me know if that works for you.

I'm having trouble reproducing the jittering issue with the emotions dynamic field, though I suspect it might have to do with offsetting the size update by one frame.

For removing current_value == "", I had a feeling the code was there for a reason. From my testing so far, removing it seems to fix the box not disappearing issue and still allows the suggestions to appear when there's a default value set, and the user focuses on the field. Alternative ways to fix this could be to suppress _on_Search_text_changed from running in the focus method after we've clicked the suggestions box, although I'm in favor of removing the line.

@Jowan-Spooner Jowan-Spooner merged commit 5c59b33 into dialogic-godot:main Feb 11, 2024
2 checks passed
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.

2 participants