search

ssokolow / quicktile

Adds window-tiling hotkeys to any X11 desktop. (An analogue to WinSplit Revolution for people who don't want to use Compiz Grid)
https://ssokolow.com/quicktile/
GNU General Public License v2.0
860 stars 78 forks source link

move-to-{top,left,right,bottom} behavior changed in 0.4.0 #116

Closed ghost closed 11 months ago

ghost commented 4 years ago

As of 0.4.0, the move-to-{top,left,right,bottom} commands now center the window in the other coordinate. For example, move-to-right will move the window to the right edge of the screen and center it vertically. In 0.3.0, the window's previous vertical position was preserved. It looks like this change was made in 2361a3e.

I personally prefer the old behavior, so I added it back as a new set of commands move-to-{top,left,right,bottom}-relative here: stygstra@c6b2976. Is this something you would be interested in accepting as a patch? If so, I can clean up the code a bit, test it more thoroughly, and submit a pull request.

ssokolow commented 4 years ago

I'm about to go to bed, so I'll need to think about it tomorrow.

However, I can say that, at least in that commit, you neglected to run docs/update_authors.sh to regenerate AUTHORS as described in the Adding Yourself to the AUTHORS List section of the Developer's Guide page of the manual.

ghost commented 4 years ago

Thanks! I'll be sure to do that before submitting a pull request. I just opened the issue to check that this is something you would be interested in adding before I put more work in; it's definitely not ready for merging yet. I haven't run any of the tests or linters, either.

ssokolow commented 4 years ago

Bear in mind that I'm about to merge the hidpi_fix branch I meant to merge a month ago before the TODO note got buried, so you'll need to rebase it.

ghost commented 4 years ago

Okay. I improved the implementation a bit, gave the commands a more descriptive name (in my opinion), and confirmed that my changes apply cleanly to the hidpi_fix branch as well as the master branch. My new branch is here, if you want to look: 116-nudge.

I went through the developer guide, as you requested earlier. Mypy, flake8, and pylint all failed, but they were also failing before. I looked at the diff of their output from before and after my changes and confirmed that I didn't introduce any new errors.

I'm ready to open a pull request for this change, but I need to know two things:

ssokolow commented 4 years ago

I went through the developer guide, as you requested earlier. Mypy, flake8, and pylint all failed, but they were also failing before. I looked at the diff of their output from before and after my changes and confirmed that I didn't introduce any new errors.

Ugh. They're not supposed to complain that much. I forgot to take my system-wide configuration defaults into account, so I didn't realize the project linting configuration was incomplete and wouldn't be consistent across different systems.

I'll try to push some fixes for that either today or tomorrow.

Are you open to adding these new commands to QuickTile? We can work out any implementation details in the pull request.

I'm not against it, but I'll need to think about it more.

I can already say, though, I think nudge- is a regression because now you have a different "meta-command" name for part of the Gravity enum, a regression in the consistency I was going for. (eg. If I ever do an API-breaking 1.0 release before Wayland kills off X11, I was planning to change it to a single move-to command that takes arguments.)

Would you like the pull request made against master or against hidpi_fix?

master. I'm hoping to merge hidpi_fix into master by the end of tomorrow.

ssokolow commented 4 years ago

OK. hidpi_fix has been merged into master and I also updated the linting instructions and run_tests.sh so they should have the same configuration on your end as on mine.

ssokolow commented 4 years ago

Marking as "needs more info" since I'm open to some kind of implementation but more discussion is needed and I'm waiting for a reply.

ssokolow commented 11 months ago

Closing because "needs more info" for over two years without receiving a reply.