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

Take into account WM decorations when tiling #66

Merged
merged 2 commits into from
Jul 23, 2016
Merged

Conversation

cqexbesd
Copy link
Contributor

Stop the xterms overlapping. Only tested under OpenBox.

Not all window managers support this but they won't be any worse
off than they are now and those that do (all recent managers IIUC)
should be better.

@cqexbesd
Copy link
Contributor Author

the tests failed because they didn't install X11::Protocol::WM. I've tried adding it elsewhere but I'm not experienced with Module::Build so please double check I have done that right.

@cqexbesd
Copy link
Contributor Author

OK now everything passes except perl 5.8 which doesn't support //=. I assume 5.8 is still a supported release so I will write it out long hand.

@cqexbesd
Copy link
Contributor Author

OK I have replaced //= with a function. I have removed the make perltidy happy commit because I note that it wants to change lots of pre-existing code, suggesting the test should already be failing. Not sure how you want to handle that...

Not all window managers support this but they won't be any worse
off than they are now.
@cqexbesd
Copy link
Contributor Author

OK, perltidy commit is back. The changes to existing code was just because I missed the -pbp flag one time.

I can merge the two commits if you prefer. Let me know how you would like it done.

@duncs duncs merged commit d3b8bc0 into duncs:master Jul 23, 2016
@duncs
Copy link
Owner

duncs commented Jul 23, 2016

Thanks for the patch

Duncs

duncs added a commit that referenced this pull request Jul 23, 2016
@harridu
Copy link

harridu commented Nov 2, 2016

That was a bad idea. Now all the xterms have gaps between them, wasting precious screen space :-(. The xterms on the lower edge are partially outside of the visible screen.

Please revert this change.

@cqexbesd
Copy link
Contributor Author

cqexbesd commented Nov 2, 2016

Have you checked your config? There are quite a few people running it here who don't see that. Perhaps you have added space around your windows before to allow for window decorations that you no longer need? What values do you have set for e.g. terminal_reserve_*?

@harridu
Copy link

harridu commented Nov 4, 2016

For testing I had used the default configuration. No ~/.clusterssh directory. No ~/.config/clusterssh, either.

Even if I did modify the layout, my private config settings should have a higher priority than the default derived from the frame size. Pull #66 should be transparent for a custom layout, shouldn't it?

BTW, window manager is fvwm. Hope this helps.

@cqexbesd
Copy link
Contributor Author

cqexbesd commented Nov 4, 2016

Even if I did modify the layout, my private config settings should have a higher priority than the default derived from the frame size. Pull #66 should be transparent for a custom layout, shouldn't it?

What the change did was correctly calculate the window size. Previously it was usually thought to be smaller than it was. I believe, though I haven't checked, that the default settings put a bit of space around each window. In actuality this space was less than it should have been as it was partially taken up by the WM decorations. As long as the WM decorations were small enough though there would be no overlap between windows. If that wasn't the case then windows would overlap which is obviously the worst outcome. I believe now they should never overlap, as long as there is enough room and the WM allows - though of course there may be bugs and layout is still not ideal.

If your configuration (whether by active choice or defaults) has space around windows that was partially filled by the WM decorations before then, now that has been fixed, you will be actually getting the space you asked for. If you don't want space maybe try setting those settings to 0 and see if that works the way you want.

It might be a few weeks before I can have a deeper look otherwise but let me know how you get on.

Thanks

Andrew

BTW, window manager is fvwm. Hope this helps.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@harridu
Copy link

harridu commented Nov 7, 2016

I am not sure if I got you correctly, but I didn't ask for space around these xterms. If the default config doesn't avoid that some xterms are partially off-screen or waste screen space, then these default values should be adjusted.

PS: Apparently there is a race condition as well. If I run cssh to open 20 xterms on localhost, then some are placed as if they don't have decoration at the left side, for example. Others appear to be moved up exactly by the size of the top edge decoration. Ctrl-r doesn't help.

You know that the window decoration might be attached to a window after it became visible?

To reproduce this you should consider to try fvwm yourself. fvwm's default configuration should do. The race condition becomes pretty obvious if you run fvwm on another host, e.g. on your laptop. Just for debugging, of course.

Hope this helps
Harri

@harridu
Copy link

harridu commented Nov 7, 2016

PPS: Here is a screenshot:
cssh_sample

@rlucassen2
Copy link

WindowMaker: exactly the same behaviour. Downgrading to 4.07 fixes the issue (Debian testing). Try to open more sessions and check if xterms are positioned outside the screen, partially visible or completely unvisible. Normally clusterssh should resize the xterms to fit onto the screen. IOW: cssh thinks the screen is bigger than it is.

@cqexbesd
Copy link
Contributor Author

cqexbesd commented Nov 7, 2016

Ok well I still can't reproduce using openbox or a few other WMs people around me are running. It will still be sometime before I get any time to install WindowMaker and try it out. That doesn't mean someone else won't in the mean time of course.

It may be you will have to use the old method of adjusting the terminal_reserve variables in the mean time. If you run cssh -d it should dump the config you are currently using.

If you were curious you could also try running the script attached to issue 68 and see if it exhibits the same problem. I hope to merge that into cssh at some point if people are agreeable (and I get some time) so if that fixes it then it saves me looking further ;-)

Andrew

@rlucassen2
Copy link

I have already played with the conf file, but the terminal_reserve variables have to be adjusted to the number of xterms called bij cssh. I also tried to remove the config file and use the default config that is wriiten by cssh when cssh starts. There is no systemwide config AFAICS. For the moment I have put clusterssh on hold in the package management as clusterssh is a high prio program over here ;-)

Richard.

@cqexbesd
Copy link
Contributor Author

cqexbesd commented Nov 7, 2016

On 7 Nov 2016, at 15:31, rlucassen2 notifications@github.com wrote:

I have already played with the conf file, but the terminal_reserve variables have to be adjusted to the number of xterms called bij cssh.

IIUC that should not be the case. It's just how much space to leave around the windows.
I also tried to remove the config file and use the default config that is wriiten by cssh when cssh starts.

Yes I believe that has a bit of space left around terminal windows but maybe you need more.

Andrew

@rlucassen2
Copy link

IIUC that should not be the case. It's just how much space to leave around the windows.

That's true. That's what it should do. But it doesn't. E.g. at 6 xtermes you need screen_reserve_bottom=10, with 8 xterms screen_reserve_bottom=60 and so on. If you don't adapt this to the number of xterms, the lowest xterms will be partially visible or completely unvisible. And this is apart from the effect that windows are overlapping randomly as you can see in the png shown above

Richard

duncs added a commit that referenced this pull request Mar 7, 2017
Previous change "Take into account WM decorations when tiling (Github pull
request #66) (thanks to Andrew Stevenson)" has caused problems on some
systems but improved tiling on others, so make the algorithm configurable
to use the original one (default) or the new style.

To be revisited when all the window handling code is moved into a separate
module
@duncs
Copy link
Owner

duncs commented Mar 7, 2017

I have applied the Debian patch by Tony Mancill which allows the use of the new algorithm to be configurable; it helps some people but doesn't help others.

Duncs

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.

None yet

4 participants