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

runelite-client: Don't try to contain to screen when contain is off #9270

Merged
merged 1 commit into from Jul 5, 2019

Conversation

abextm
Copy link
Member

@abextm abextm commented Jul 3, 2019

Java's GraphicsConfiguration::getBounds() is completely nonfunctional for me, returning nearly random values, which causes the our window to move itself to random places, often outside of my displays' bounds

Java's GraphicsConfiguration::getBounds() is completely nonfunctional
for me, returning nearly random values, which causes the our window to
move itself to random places, often outside of my displays' bounds
@abextm abextm added the bug label Jul 3, 2019
@deathbeam
Copy link
Member

Im pretty sure this issue is global to most unix window managers. I think this issue is related: #2561

@abextm
Copy link
Member Author

abextm commented Jul 3, 2019

#2521 is because it awt thinks the window has a ~20px titlebar always on x11, and tries to move the content pane to be that far down from the window's bounds. This is some other (probably unrelated) x11 java bug.

@deathbeam deathbeam merged commit d366521 into runelite:master Jul 5, 2019
@themrrobert
Copy link

themrrobert commented Jul 9, 2019

I do not like the changes to ContainableFrame.java in d72a1b9

When my window is on the right of the screen, which it always is, the frame now pops up off screen to the right. The old behavior is restored when enabling Contain to screen, but this is undesirable as I frequently move the client off-screen in order to work.

If this is a workaround to an x11 bug, then perhaps an additional option should be provided for that, as (for me) this breaks existing functionality that I was using. I just started learning the codebase so I don't have all the facts to offer a better solution.

Thanks :)

/edit: i'm just adding my opinion, not demanding anything, hopefully that came across properly

@Adam-
Copy link
Member

Adam- commented Jul 10, 2019

For me, this causes the client to expand out of bounds. How is this helping you if it is still using getGraphicsConfiguration().getBounds()?

@abextm
Copy link
Member Author

abextm commented Jul 10, 2019

getGraphicsConfiguration().getBounds() is locked behind a config, which I have disabled, so it's values don't get used.

@Adam-
Copy link
Member

Adam- commented Jul 10, 2019

The config option is meant to prevent dragging the client off screen. Using it to determine if the client expanding should be allowed to expand off screen seems unrelated. Generally I think when someone expands the client without the client locked to the screen the expected behavior would not be the client expanding off screen.

To me this seems like a major UX regression. If your problem is truly specific to *nix WMs I would rather see a different approach taken since so few users use nix.

Have you tried either trying to sanity check the values to determine if they are bogus, or at the very least just disable the window expand logic on non-Windows/osx?

@abextm
Copy link
Member Author

abextm commented Jul 10, 2019

The config option is described as Makes the client stay contained in the screen when attempted to move out of it., which applies to the client resizing or being moved by the user. If you want we can reword the config option to what you think it should be, and replace this check with an OS == Linux check instead, because as far as I can tell most of openjdk's X11 code is mostly unmaintained and not supported.
Additionally the config description contains Note: Only works if custom chrome is enabled. which seems to be wrong because this does atleast attempt to work without custom chrome on.

@Adam-
Copy link
Member

Adam- commented Jul 10, 2019

It looks like the call to setContainedInScreen checks the titlebar, so it doesn't run. Not sure why.

The behavior of the release is that it will expand and keep itself in the bounds of the current screen, even when you have multiple screens, with the config option off (which is default)

@deathbeam
Copy link
Member

Generally I think when someone expands the client without the client locked to the screen the expected behavior would not be the client expanding off screen.

I think its exactly opposite. This behaviour should be at least configurable, and now it is. If people want to split it, they should open new issue for that. NFC why is this being discussed on PR.

It looks like the call to setContainedInScreen checks the titlebar, so it doesn't run. Not sure why.

The setContainedinScreen checks the titlebar, because without custom window chrome (e.g no "title bar"), OS should handle everything window related, and not this dumb stuff we are doing with Swing.

@Adam-
Copy link
Member

Adam- commented Jul 11, 2019

The previous behavior of:

  1. not locking moving to any one screen
  2. moving the client to not expand off of the current screen

is no longer possible now. I think this is a good default behavior of the client and should be kept as the default behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants