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

Implement custom split size #2564

Closed
wants to merge 11 commits into from

Conversation

idr4n
Copy link

@idr4n idr4n commented Nov 1, 2024

Hi @mitchellh,

This is my take on custom split sizes, as referred in #2227, for your review and consideration. Bare in mind that this is my first time working in Zig 😅.

Known issue:

  • The split size calculation works as expected in displays that are not scaled. I tested this in Arch linux, Hyprland, and the percentage is applied properly if the display is not scaled. If the display is scaled, then the custom size is not properly calculated for some reason I couldn't find out yet. Any ideas on this? Besides sibling.size in src/apprt/gtk/Split.zig, I tried sibling.core_surface.screen_size or playing around with grid_size and cell_size but still got the same issue. Update: this has been fixed now with getContentScale.

Unresolved problem:

  • macOS implementation.

Tests:

  • Added tests in src/input/Binding.zig ("parse: action with enum") for parsing the new action flag and tests are passing.

Update:

Custom size splits work in gtk and macOS (with the known issue for gtk above but not for macOS).

@@ -454,6 +454,13 @@ pub const Action = union(enum) {
auto, // splits along the larger direction
};

pub const Percentage = []const u8;
Copy link
Member

Choose a reason for hiding this comment

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

This should really be a numeric parameter. I'd suggest a f32 that is clamped to 0.0 <= pct <= 1.0. That way the percentage doesn't have to be re-parsed every time a window is split.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I agree, thanks for pointing that out @jcollie. I tried initially to set is as u16 but then was having issues parcing the suggested syntax "new_split:right,30%" to invalidate something like "new_split:right,30" just for new splits while keeping valid for other actions.

After your comment, I tried a couple of other things, and got this so far. How about setting Percentage as a Struct instead, such as:

    pub const Percentage = struct {
        value: u16,

        pub fn init(value: u16) Percentage {
            return .{ .value = value };
        }
    };

and then in the parseParemeter logic add a Struct in the switch statement, such as:

.Struct => if (field_.type == Percentage) try parsePercentage(field_.type, next),

The parcePercentage function would be something like:

    fn parsePercentage(comptime T: type, value: []const u8) !T {
        if (value.len < 2) return Error.InvalidFormat;
        if (value[value.len - 1] != '%') return Error.InvalidFormat;
        const percent = value[0 .. value.len - 1];
        const val = std.fmt.parseInt(u16, percent, 10) catch return Error.InvalidFormat;
        return Percentage.init(val);
    }

I found that this is working and passing the corresponding tests, as well as differentiating actions such as resize_split:up,10 that require an int, from new_split:left,30% that should enforce an int ending with %.

Copy link
Author

@idr4n idr4n Nov 7, 2024

Choose a reason for hiding this comment

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

I have changed Percentage to f32, which is clamped between 0 and 1 in the latest commit, and parameter parsing logic is being handled by parseFloat instead, so the newly added parsePercentage has been removed.

@idr4n idr4n requested a review from jcollie November 7, 2024 20:42
@idr4n idr4n changed the title DRAFT: implement custom split size Implement custom split size Nov 10, 2024
@idr4n
Copy link
Author

idr4n commented Nov 10, 2024

In the last commits I changed from f32 to u16 to avoid some extra hashing when parsing the flags (which somehow was needed when using a float.). I think u16 leads to a cleaner approach, imho. At the end, obviously the design choice is entirely yours and I could see this not being merged as it involves a lot of changes and design decisions. However it was a fun and nice learning experience for me. At this point I think this is as far as I can go with this PR as the issue with gtk and scaled displays I encountered, which most likely is related to OpenGL, is way out of my scope The initial issue with scaled displays in gtk has been resolved as well (it was an easy fix actually with getContentScale). For macOS and gtk is working nicely on my side. Therefore, I removed the DRAFT from the title and put it up to your consideration @mitchellh. Thanks in advance!

@idr4n idr4n closed this Feb 28, 2025
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.

2 participants