quicktile icon indicating copy to clipboard operation
quicktile copied to clipboard

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

Open ghost opened this issue 5 years ago • 7 comments

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.

ghost avatar May 09 '20 01:05 ghost

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.

ssokolow avatar May 09 '20 02:05 ssokolow

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.

ghost avatar May 09 '20 02:05 ghost

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.

ssokolow avatar May 09 '20 02:05 ssokolow

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:

  • Are you open to adding these new commands to QuickTile? We can work out any implementation details in the pull request.
  • Would you like the pull request made against master or against hidpi_fix?

ghost avatar May 09 '20 07:05 ghost

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 avatar May 09 '20 19:05 ssokolow

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 avatar May 11 '20 02:05 ssokolow

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 avatar May 20 '20 08:05 ssokolow

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

ssokolow avatar Aug 08 '23 16:08 ssokolow