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

Resize tile px #3240

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
4 participants
@andrewla
Copy link

commented Apr 13, 2018

Allows resizing of tiled containers by px instead of just by ppt. Syntax is just appended to current resize command, and will be backwards compatible if px is not specified.

Fixes #3239

@orestisf1993

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

Can you address the failing tests?

@andrewla

This comment has been minimized.

Copy link
Author

commented Apr 24, 2018

Took a bit of debugging to figure out why it wasn't resizing correctly in this case, and some learning about how the actual layout code works, rather than just assuming magic.

@andrewla

This comment has been minimized.

Copy link
Author

commented Apr 24, 2018

NB: I have fixed existing tests, but have not added anything to test new functionality. Now that I have all the testing framework working on my machine (kudos to perlbrew!) instead of using Travis I should be able to operate faster.

That said, the backwards compatibility of the change is well-demonstrated here; as long as both ppt and px are specified the behavior is unchanged.

@orestisf1993

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

AFAIK, we can't test this change

@andrewla

This comment has been minimized.

Copy link
Author

commented Apr 24, 2018

I didn't mean to imply that I expected anyone else to write tests for this change, merely saying that I don't think this change is ready to merge until I write some tests covering the new functionality.

That's assuming that you see fit to accept the change at all; #3239 covers my justification for having made the change. I like the change as well because it simplifies some of the resizing logic even for the ppt case.

@orestisf1993

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

@orestisf1993 orestisf1993 removed the waiting label Apr 25, 2018

Adding tests for pixel tiled resizing
This testing exposed an off-by-one bug in the resizing code in certain
cases that I'm still investigating.
@Airblader

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

@andrewla Can you address the failing tests please?

@Airblader Airblader added the stale label Jun 22, 2018

@andrewla

This comment has been minimized.

Copy link
Author

commented Jun 28, 2018

Sorry, this has been on my back burner. I fixed the existing test, but during testing of new functionality I uncovered a bug that I had some trouble hunting down -- namely that resizing in a shrinking direction does so by one less pixel than it should, due to rippling rounding errors caused by the way that window sizes are calculated.

@michaelnew

This comment has been minimized.

Copy link

commented Aug 22, 2018

Sorry to be that guy, but... any progress on this? It seems awfully close and it would be such a shame to not get it merged. This is a feature I've been really missing for a long time.

orestisf1993 added a commit to orestisf1993/i3 that referenced this pull request Aug 23, 2018

Introduce parse_direction
Also fixes the following bug: in the fix for i3#1011 in
cmd_resize_floating direction "width" is not considered.

Influenced by i3#3240.

orestisf1993 added a commit to orestisf1993/i3 that referenced this pull request Aug 23, 2018

Introduce parse_direction
Also fixes the following bug: in the fix for i3#1011 in
cmd_resize_floating direction "width" is not considered.

Influenced by i3#3240.

orestisf1993 added a commit to orestisf1993/i3 that referenced this pull request Aug 23, 2018

Introduce parse_direction
Also fixes the following bug: in the fix for i3#1011 in
cmd_resize_floating direction "width" is not considered.

Influenced by i3#3240.

orestisf1993 added a commit to orestisf1993/i3 that referenced this pull request Aug 24, 2018

render.c: round sizes instead of flooring them
This will lead to more accurate and consistent container sizes.

Needed to fix the failing test of i3#3240.

orestisf1993 added a commit to orestisf1993/i3 that referenced this pull request Aug 24, 2018

Make cmd_resize_tiling_direction work with pixels
Introduces resize_neighboring_cons in resize.c which is also used by
resize_graphical_handler.

Co-authored-by: Andrew Laucius <andrewla@gmail.com>
Authored original code and tests in i3#3240. I rewrote most of the
resizing code and fixed the failing tests.

@orestisf1993 orestisf1993 referenced this pull request Aug 24, 2018

Merged

Resize tile px #3372

@orestisf1993

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

I am continuing this in #3372 with support for other commands as well.

orestisf1993 added a commit to orestisf1993/i3 that referenced this pull request Aug 24, 2018

precalculate_sizes: round sizes instead of flooring them
This will lead to more accurate and consistent container sizes.

Needed to fix the failing test of i3#3240.

orestisf1993 added a commit to orestisf1993/i3 that referenced this pull request Aug 24, 2018

Make cmd_resize_tiling_direction work with pixels
Introduces resize_neighboring_cons in resize.c which is also used by
resize_graphical_handler.

Co-authored-by: Andrew Laucius <andrewla@gmail.com>
Authored original code and tests in i3#3240. I rewrote most of the
resizing code and fixed the failing tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.