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

Dragging/sizing main menu pushes it back into screen rect #849

Merged

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Apr 17, 2020

Fixes #848
Fixes #857

Minor modification addressing comment in #819 by @kianzarrin
Removed commented code which will never be used.

CHECKLIST

  • Update tooltip for remaining sliders (one?)

@originalfoo
Copy link
Member

Linked #848

@originalfoo originalfoo added Accessibility Color blindness, etc. Toolbar The main TMPE toolbar UI User interface updates Usability Make mod easier to use labels Apr 17, 2020
@originalfoo originalfoo added this to the 11.5.0 milestone Apr 17, 2020
@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 17, 2020

Somehow the actual main menu never goes offscreen, even though i react on draghandle size going off screen, it is always entirely in-screen. I suspect its the dragging code in CO.UI which limits the window into the screen rect.

@originalfoo
Copy link
Member

Making it go off-screen:

  1. Set scaling factor small
  2. Drag window to edge of screen
  3. Increase scaling factor - top-left of menu stays in same place, but now it's bigger so some of it is off-screen

So maybe recheck bounds when scaling is changed?

Also, (I haven't tried this yet) what happens if menu is dragged towards bottom right of screen, then the screen resolution is reduced? Will it be completely off-screen?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 17, 2020

If you drag to bottom right (i tried that), the main menu remains in GUI coordinate grid, which is always 1920x1080, so it does not move from the corner and is fully in screen.
And yes, i apply the clamp code to the main menu when UI scale changes.

@kianzarrin
Copy link
Collaborator

If you drag to bottom right (i tried that), the main menu remains in GUI coordinate grid,

Yes. I you want to push the GUI outside of screen you have to hack your way into it as aubergine explained.

And yes, i apply the clamp code to the main menu when UI scale changes.

Then why does it go outside of screen? maybe call refresh or invalidate or something. do you refresh absolute position?

@kianzarrin
Copy link
Collaborator

Can you call RefreshTooltip() on the sliders in the Options so that as the user moves the slider, the tooltip value gets updated.:

protected override void OnValueChanged() {
	base.OnValueChanged();
	tooltip = value.ToString();
	RefreshTooltip();
}

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 18, 2020

@kianzarrin addressed your list of forgotten commented fragments, they are removed
Thank you for that.
Also: Adding update tooltips and considering how to make this into every slider that we have. This works so well!

Copy link
Collaborator

@kianzarrin kianzarrin left a comment

Choose a reason for hiding this comment

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

thanks for removing the commented out code and cool sliders!

It does not push back the the main menu into screen:
Screenshot (889)

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 19, 2020

The push logic only forces the label (and drag handle) to return back to the screen, not entire window.
The drag handle logic (C:S own code) seems to keep entire window in screen.
Can you make the drag handle off-screen at least partially?

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

👍

@originalfoo originalfoo linked an issue Apr 21, 2020 that may be closed by this pull request
@originalfoo
Copy link
Member

Loving the sliders updating their tooltips!

However, I was able to make the TMPE menu disappear in a non-recoverable fashion, although I'm not sure how many users will encounter this:

image

With the menu dragged to bottom right corner of screen, I changed from 16:9 2560x1440 resolution down to 4:3 640x480 resolution. Somewhat extreme (just to test), and obviously this isn't going to happen very often IRL. I didn't tested yet for less extreme aspect ratio and resolution changes.

However, some users with low spec systems often run on very low graphics most of the time, but then jump up to higher resolutions to take nice screenshots, so the issue described here it's not completely impossible to happen in the wild.

My suggestion is this - rather than trying to fix this extreme case, why not build a little "recovery mode" in to the main TMPE button?

Specifically:

Double click the 'crown button' to reset the toolbar position to 0, 0.

This way if we ever get users saying their toolbar disappeared, we can just reply "double click the tmpe button" (as that is usually still on screen based on past reports of missing toolbar).

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

See comment above.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 22, 2020

Need to double check the assumption again: whether the GUI remains 1920x1080 units internally even when you change the aspect. In the code I assumed that it does. The facts might be telling otherwise.

@kianzarrin
Copy link
Collaborator

@kvakvs a bit outside of the scope of this review but there is only one slider left without the tooltip refresh and then we can close #857 that's the "dynamic lane selection" in GamePlay Tab

@kianzarrin
Copy link
Collaborator

Sorry I got a bit lost in the long conversations above. @kvakvs are you planning to push the whole window back into screen when the size gets larger. I have tracked the code and I can see you have observers that should push the window back into screen. but for some reason they don't. maybe put some breakpoints to see if the code is actually being executed?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 27, 2020

The code is pushing the window back so that the handle becomes fully visible. Just what's necessary to drag the window. Does it actually work? I am not sure, all my screen resolutions had same aspect ratio and always 1920x1080 GUI resolution.

@originalfoo
Copy link
Member

I think this only needs that slider tooltip refresh doing (DLS slider in gameplay tab iirc) then it is good to merge.

@originalfoo
Copy link
Member

@kianzarrin What does that stuff above have to do with toolbar? Did you comment on the wrong PR?

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kvakvs kvakvs merged commit 5eff305 into CitiesSkylinesMods:master Apr 30, 2020
@originalfoo originalfoo linked an issue May 2, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Color blindness, etc. Toolbar The main TMPE toolbar UI User interface updates Usability Make mod easier to use
Projects
None yet
4 participants