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

Pr/maximum size #475

Open
wants to merge 7 commits into
base: 3.6.x
Choose a base branch
from
Open

Pr/maximum size #475

wants to merge 7 commits into from

Conversation

uli42
Copy link
Member

@uli42 uli42 commented Jul 3, 2017

Set the limit for maximum screen size to 32767x32767

Please test if this works with your favourite window manager.

Copy link
Member

@sunweaver sunweaver left a comment

Choose a reason for hiding this comment

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

uli42: I don't think that the fix you propose here is the right solution to the underlying problem. I am not even sure, if what you saw as a problem really is a problem, or rather the problem.

We need to discuss this further, maybe we need some screen casts, that demonstrate the problem.

@@ -81,7 +81,6 @@ RRScanOldConfig(ScreenPtr pScreen, Rotation rotations)
RRModePtr mode, newMode = NULL;
int i;
CARD16 minWidth = MAXSHORT, minHeight = MAXSHORT;
CARD16 maxWidth = 0, maxHeight = 0;
CARD16 width, height;
Copy link
Member

Choose a reason for hiding this comment

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

@uli42: I don't really like it that we have to mess around with code in Xserver/randr/. Can't this be addressed insider the nxagent code itself?

I wonder, if this could be approached differently by looking more deeply at nxagentRandRInitSizes() (in hw/nxagent/Extensions.c) so that we allow for more screen sizes initially. (Just a guess). Also, I wonder why RRGetInfo actually gets called as we have an override function named nxagentRandRGetInfo() (also in Extensions.c).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, need to investigate.

@@ -639,9 +639,6 @@ void nxagentShadowSwitchResizeMode(ScreenPtr pScreen)
nxagentShadowCreateMainWindow(screenInfo.screens[DefaultScreen(nxagentDisplay)], screenInfo.screens[0]->root,
screenInfo.screens[0]->root -> drawable.width, screenInfo.screens[0]->root -> drawable.height);

sizeHints.max_width = nxagentOption(RootWidth);
sizeHints.max_height = nxagentOption(RootHeight);

Copy link
Member

Choose a reason for hiding this comment

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

Same here... The WM_NORMAL_HINTS property should always match the current situation of the screen layout, IMHO.

sizeHints.max_width = WidthOfScreen(DefaultScreenOfDisplay(nxagentDisplay));
sizeHints.max_height = HeightOfScreen(DefaultScreenOfDisplay(nxagentDisplay));
sizeHints.max_width = MAXSHORT;
sizeHints.max_height = MAXSHORT;

Copy link
Member

Choose a reason for hiding this comment

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

Have you actually testing the above code in nxagentShadowAdaptToRatio()?

Copy link
Member Author

Choose a reason for hiding this comment

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

honestly, no.

@uli42
Copy link
Member Author

uli42 commented Jul 10, 2017

For clarification about what this patch addresses: I normally connect from different clients to my long-running session. Some clients have two monitors, some have one, and also the resolutions differ. As long as the initial session start happened on the client with the biggest resolution I have no problems. But if I happen to start (not reconnect!) the session from a "smaller" client that client's resolution is the value that shows up in xrandr. Reconnecting from a "bigger" client leaves me with a session that is not extendable to the full resolution. As I am using fullscreen sessions this leaves me with some black bars of unusable screen estate with no way to do something against it.

@uli42 uli42 force-pushed the pr/maximum_size branch from 603e542 to 8eb3531 Compare July 11, 2017 22:05
@uli42 uli42 dismissed sunweaver’s stale review July 12, 2017 12:34

Have pushed a new version that is less intrusive and also works if DesktopResizeMode is off

@uli42
Copy link
Member Author

uli42 commented Jul 12, 2017

Please review once again

Copy link
Member

@sunweaver sunweaver left a comment

Choose a reason for hiding this comment

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

Two more comments...

Also, while at it, shouldn't we add typical resolutions up to 8k here to nxagentRandRInitSizes() right away?

@@ -284,9 +284,9 @@ static int nxagentRandRInitSizes(ScreenPtr pScreen)
1024, 768, 900, 900, 1200, 1050, 1080, 1200, 0, 0};
*/
int w[] = {0, 320, 640, 640, 800, 800, 1024, 1024, 1152, 1280, 1280, 1280, 1360,
1440, 1600, 1600, 1680, 1920, 1920, 0, 0};
1440, 1600, 1600, 1680, 1920, 1920, 32767, 0, 0};
Copy link
Member

Choose a reason for hiding this comment

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

Why not MAXSHORT here instead of 32767?

int h[] = {0, 240, 360, 480, 480, 600, 600, 768, 864, 720, 800, 1024, 768,
900, 900, 1200, 1050, 1080, 1200, 0, 0};
900, 900, 1200, 1050, 1080, 1200, 32767, 0, 0};
Copy link
Member

Choose a reason for hiding this comment

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

Same here...

Copy link
Member Author

Choose a reason for hiding this comment

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

MAXSHORT ist better, agreed. We can add some more resolutions but with the new maximum size you can add any resolution on your own.

However: As long as rrxinerama is active your won't ever see those resolutions.

@sunweaver
Copy link
Member

sunweaver commented Jul 12, 2017 via email

@uli42 uli42 force-pushed the pr/maximum_size branch from 8eb3531 to 28139e7 Compare July 12, 2017 21:04
@sunweaver
Copy link
Member

@uli42: please rebase against latest 3.6.x branch, so I can merge.

@sunweaver
Copy link
Member

@uli42: What is the status of #475? I already approved the merge, but you added more commits without comment. Was that by accident? It feels that some of the newer commits should be part of a second PR.

Please give feedback on your intention for this PR. Thanks!

@uli42
Copy link
Member Author

uli42 commented Aug 30, 2017

To answer your question: no, those commits where not by accident, I simply noticed that we need them in the -rrxinerama case.

This patch is working but kind of worthless. It revealed a general problem: with rrxinerama activated (which is the default) modifying screens via xrandr is not really working well due to the fact the the xinerama code will throw away what the user specified on the next move or resize of the nx window. I have not yet come up with a proper idea how to handle that. You can, however, use this code together -rrxinerama.

@uli42 uli42 mentioned this pull request Dec 13, 2017
Set maximum size to MAXSHORT (32767).

This especially helps when reconnecting to a session that had
initially been started from a client with a small(er) screen. Before
it was impossible to increase the maximum size of a running session
beyond the screen size of the client that had initiated the session.

This change, together with the previous commit, fixes ArcticaProject#472
place the maximum value directly in the resolution list.
@sunweaver
Copy link
Member

@Ionic: can you take a second look at that (with my look being the first). I've stumbled over this quite often when switching sessions between machines. Esp. when switching to machines with 4k displays, the size limitation of the previous host is nasty. I'd love to get this changeset in.

@uli42
Copy link
Member Author

uli42 commented Jul 23, 2018

sunweaver: so you saw this limitation even with active rrxinerama?

@sunweaver
Copy link
Member

@uli42: @Ionic: we should get this PR merged over the weekend, IMHO.

@sunweaver
Copy link
Member

@uli42: @Ionic: this PR #475 has become quite stale but is still an issue in real life cases. I am inclined to rebase and merge it. Thoughts? Vetos?

@sunweaver
Copy link
Member

What about this one? The maximum size issue is some sort of a problem, indeed. I was about to rebase it, but then spotted the top commit on this branch. Please cleanup (and rebase).

@uli42
Copy link
Member Author

uli42 commented Jan 5, 2020 via email

@sunweaver
Copy link
Member

must check - but not today

Gentle reminder...

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.

2 participants