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

split ratio, split horizontal factor, split vert factor #44

Merged
merged 7 commits into from
Feb 8, 2024

Conversation

JoseConseco
Copy link
Contributor

@JoseConseco JoseConseco commented Nov 6, 2022

split ratio - for example 2 => window has to be 2x wider than normal, before left right split is done.
split widht - 0.8 => reduce size of new window 20% when left/right split
split height - similar to above for top/bottom splits

    parser.add_argument("-sw",
                        "--splitwidth",
                        help='set the width of the vertical split (as factor); default: 1.0;',
                        type=float,
                        default=1.0, )
    parser.add_argument("-sh",
                        "--splitheight",
                        help='set the height of the horizontal split (as factor); default: 1.0;',
                        type=float,
                        default=1, )
    parser.add_argument("-sr",
                        "--splitratio",
                        help='Split direction ratio - based on window height/width; default: 1;'
                        'try "1.61", for golden ratio - window has to be 61% wider for left/right split; default: 1;',
                        type=float,
                        default=1, )

@JoseConseco JoseConseco changed the title split ration, split horizontal split, split vert split split ratio, split horizontal factor, split vert factor Nov 6, 2022
Copy link
Owner

@nwg-piotr nwg-piotr left a comment

Choose a reason for hiding this comment

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

  1. Unescaped % character (156) makes autotiling -h crash. Please use %% instead.
  2. As well -sw as -sh & -sr are floats. Shouldn't they all have the 1.0 default value?

After short testing: I don't think I would use any of the new arguments. But I may be wrong, of course. @Syphdias, what do you think?

@JoseConseco
Copy link
Contributor Author

Fixed the issues. This PR is mostly cosmetic, its kind of nice to have golden ration split rule. It can help reduce amount of window resizing, if user has some specific workflow needs (like me - I prefer slight bias toward top/bottom splits, and I prefer to have bigger 'Mater' window, thus I can have smaller child splits now). Also this kind of helps to get spiral layout. User does not have to use it if you like 'normal/unifom' splits.

@Syphdias
Copy link
Contributor

Syphdias commented Nov 8, 2022

Funny, I already coded up the split ratio a few days ago in a very similar manner and already use it with --ratio 1.25. So yes, this I do find very useful. It might help with #29.
I find it very difficult to describe this feature in few words and I think @JoseConseco is on the right track.

The other two options are interesting. I don't think I would use them though. I would try to avoid running the resize in the default case though. The help text is not very clear to me, I had to look at the code to understand what is did.
I don't think it changes the behavior but the resize calls seem necessary. Maybe default to 0.0 as disabled and add a condition if splitwidth before executing?

Suggestion for more readability. For me this makes more sense since you can see the ratio being built:

-new_layout = "splitv" if con.rect.height > 1/splitratio*con.rect.width else "splith"
+new_layout = "splitv" if con.rect.width/con.rect.height < splitratio else "splith"

@nwg-piotr
Copy link
Owner

nwg-piotr commented Nov 8, 2022

After some further testing: I could use --splitwidth and --splitheight.

@nwg-piotr
Copy link
Owner

So finally: this may be merged, but'd I'd like to merge #42 first, as it's older. @Syphdias, take a look at minor corrections I asked for, please.

@Syphdias
Copy link
Contributor

Syphdias commented Nov 8, 2022

Small heads-up, no matter which PR will be merged first, it will result in a merge conflict. At least on the handler and the function definition.

@JoseConseco
Copy link
Contributor Author

ok, all should be ok - I synced my fork and pushed Merge commit.

@nwg-piotr
Copy link
Owner

Guys, I do hope you test all the stuff against i3, do you? I have no working i3 installation at the time.

@nwg-piotr
Copy link
Owner

nwg-piotr commented Nov 9, 2022

@JoseConseco The --splitwidth & --splitheight arguments work for new windows, but not to what we move between workspaces. How about replacing

if e.change == "new" and con.percent:

with

if e.change in ["new", "move"] and con.percent:

?

@Syphdias
Copy link
Contributor

Syphdias commented Nov 9, 2022

@nwg-piotr, did you try that without a limit?

@Syphdias
Copy link
Contributor

Syphdias commented Nov 9, 2022

At least for me moving windows behaves quite weird.
moving-left
After testing autotiling is off, I open 2 more terminals and try to move the last one left and it takes quite a while.

@Syphdias
Copy link
Contributor

Syphdias commented Nov 9, 2022

Even worse if no splitwidth is set:
moving-left-nosw

@nwg-piotr
Copy link
Owner

nwg-piotr commented Nov 10, 2022

Moving windows with keyboard or i3-msg/swaymsg command behaves oddly one way or another w/ autotiling turned on. One needs to change focus first. Another way (at least working on sway) is Mod1 + mouse drag. Dunno how many users use the mouse, however.

@Syphdias
Copy link
Contributor

I can only speak for myself. Mouse dragging is quite new in i3 and I only use it sometimes, for example if I drag something into deeper structures.
I still like the manual control and I would miss it dearly even though it can be odd at times.

I still stand by my point that there should be no resizing if the option is not set (default 0.0 and check if splitwidth: etc.
If you want to the resize you can set it to 1. then it only degrades manual moving if one of the options is set.

@nwg-piotr
Copy link
Owner

nwg-piotr commented Nov 10, 2022

I still stand by my point that there should be no resizing if the option is not set (...)

Default in this case is 1.0, but you're right: it should not work when unnecessary. However, at least on sway and my version of the script, it doesn't change much: you still can not move just created window. You need to change focus first. But it has always been like this since sway 1.6. Earlier it worked as expected.

                if e.change in ["new", "move"] and con.percent:
                    if con.parent.layout == "splitv":  # top / bottom
                        if output_name in settings["autotiling-output-splitheights"] and \
                                settings["autotiling-output-splitheights"][output_name] != 1.0:
                            i3.command("resize set height {} ppt".format(
                                int(con.percent * settings["autotiling-output-splitheights"][output_name] * 100)))
                    else:  # left / right
                        if output_name in settings["autotiling-output-splitwidths"] and \
                                settings["autotiling-output-splitwidths"][output_name] != 1.0:
                            i3.command("resize set width {} ppt".format(
                                int(con.percent * settings["autotiling-output-splitwidths"][output_name] * 100)))

@JoseConseco
Copy link
Contributor Author

I'm new to i3 so I do not feel like I can say how things should work. Feel free to modify this in any way u want.
Ignoring splitwidth if == 1, sounds reasonable (If I understand it correctly my change is affecting windows width, even though it should not when sw or sh == 1, right? ).

@Syphdias
Copy link
Contributor

If I understand it correctly my change is affecting windows width, even though it should not when sw or sh == 1, right?

Yes.

I'm new to i3 so I do not feel like I can say how things should work.

We are having an open discussion. Anybody who has an opinion, can voice it. Some things we agree on, some things we disagree on. It's part of the process and hopefully the result is a better tool for all.

@nwg-piotr
Copy link
Owner

nwg-piotr commented Nov 11, 2022

To make things even harder, the repo owner does not use i3 at all. 😜 Since I moved to hybrid AMD graphics, X11 detects my outputs on the random basis, and usually I need to restart i3 3 or more times before it starts working properly.

Autotiling is just a small gear in the nwg-shell project, and I wish I had someone to keep a careful watch on it. For the project purposes I use the nwg-autotiling, script, as an entry point of nwg-shell-config. It's aimed exclusively at sway, and drops most of startup arguments (uses a common json config file), but shares the code base with autotiling.

To sum all the above up: we need discussion and a lot of testing.

@JoseConseco
Copy link
Contributor Author

Can autotile create container only after new Window is created?
The way autotile works now, vertical/horizontal containers are created in advance (eg when selecting window), we end up with too many containers that just makes managing the layout hard.
So often instead of 3 windows in one container next to each other
[A B C]
we end up with
[A [B C]]
And if we select C we end up with:
[A [B [C]]] - but visually it can be just container.
I guess I3 has to 'know' split direction before new window pop up right? I3 devs should have used bool flag for next split
direction, rather than create containers before even new window is created...

@Syphdias
Copy link
Contributor

The fact that the new split is always set when you focus to a container, breaks a couple of things. Moving a container becomes a bit odd, swiching to tabbed/stacked is not possible because it will not switch the right container (for [A [B [C]]] we want the BC-Container to be switch to stacking, but instead we switch the C-Container. The limit option somewhat mitigates that.

I guess I3 has to 'know' split direction before new window pop up right? I3 devs should have used bool flag for next split
direction, rather than create containers before even new window is created...

I cannot be a bool though, because no new layout is also valid. [A [B C D]] should sill be possible. There can be more than two containers in another container.

I would really like it, if there was a way to do it. I don't know if it is possible though. I fear i3 only knows of the new window after it is already there and you would need to go to the previously focused window, set a layout, and move the new window to it.

@JoseConseco
Copy link
Contributor Author

JoseConseco commented Nov 13, 2022

Exactly, current autotille causes to many sub containers. Its hard to manage. Visually we have 3 windows next to each other, but they may be in 3 containers, rather than one. Its pain to clean this up manually.

If have Top/Down split (green) and selecting any A, or B window adds immediately Right/Left container (red)
image
Visually nothing changed between left and right image, but we have 2 more containers - since they are added before we even add window C. This causes great deal of confusion.
Solution: only add Red sub containers, only after window C is added. When we end up with clean:
image
The algo should not be that hard - something like:

if event.win_new : 
   total_widht = C.widht + B.width  # is there way to get old selected win B? Maybe prev.sibling == B? 
   if   height > total_widht: pass (since already in top/bottom container) 
   else :    add.left_right_split(),  put B,C inside.

On top of that on window.close event, container with only one child could/should be removed (split makes no sense if there is only one object inside). Will see if I will have time to implement these, but for now I gave up on I3, since I was wasting too much time on containers management.

@Syphdias
Copy link
Contributor

Will see if I will have time to implement these, but for now I gave up on I3, since I was wasting too much time on containers management.

Will you finish the PR though?

@nwg-piotr
Copy link
Owner

Guys, I'm not going to insist on merging or further work on this PR, at least until we have an agreement. Probably I'll add --splitwidth / --splitheight to nwg-autotiling, which is a part of nwg-shell. I need to decide soon, as other changes are waiting, and I'd like not to multiply branches. Looking forward to your further opinion, if it comes to the autotiling script itself.

@JoseConseco
Copy link
Contributor Author

The change is - if e.change in ["new", "move"] and con.percent: - adding move ?

@nwg-piotr
Copy link
Owner

The change is

Also clauses in order not to resize set height if splitheight == 1.0 and resize set width if splitwidth == 1.0.

@JoseConseco
Copy link
Contributor Author

Ok, I pushed the ignore resize if == 1, and added move event. Any thoughts about remaking autotile - to only do the split after the new window is added?

  • is that not additional containers are added before new window is created,
  • users defined splitv or splith direction will be ignored - since split would be dynamic - on new window event.

@nwg-piotr
Copy link
Owner

Any thoughts about remaking autotile - to only do the split after the new window is added?

Do you have any example code to look at?

@Syphdias
Copy link
Contributor

I wrote a prototype. https://github.com/Syphdias/autotiling/blob/experimental-switching/autotiling/main.py

It does not support any arguments yet.

@Syphdias
Copy link
Contributor

Did any of you get a chance to try it out? You need to provide the --experimental flag. Most options will be ignored though.

@JoseConseco
Copy link
Contributor Author

Nope, sorry. I'm low on time. I figured I will move to https://github.com/hyprwm/Hyprland since it already has all the features I want (dynamic binary tree partitioning, tabs, good mouse support - for swapping resizing windows, nicer ui, easier to use etc).

@nwg-piotr
Copy link
Owner

Sorry, I'm working on two different things at he same time. Will take a look ASAP.

@Tmpecho
Copy link
Contributor

Tmpecho commented Aug 21, 2023

IMO the feature looks good. I don't have time at the moment to test to much extent, however @nwg-piotr you said you had implemented a similar feature before? If it worked then, it's probably good here as well.

@nwg-piotr
Copy link
Owner

I use exactly this solution in nwg-autotiling, but I was afraid that merging it here could raise one more shitstorming, as this script is quite widely used. One day I'll buy a 6-pack and make a decision. ;)

@Tmpecho
Copy link
Contributor

Tmpecho commented Aug 21, 2023

Hahah that sounds good :)

@nwg-piotr nwg-piotr merged commit 82f0264 into nwg-piotr:master Feb 8, 2024
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.

4 participants