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

Add move and grow methods to move and grow (or shrink) windows #5

Merged
merged 8 commits into from
Oct 26, 2018

Conversation

matthewfallshaw
Copy link

Major changes:

  • add move and grow methods to move and grow (or shrink) windows
  • fullscreen starts by centering the window, then proceeds through the
    sequence
    Minor changes:
  • update hs.grid when obj.GRID changes, making obj:init() unnecessary
  • forget obj._pressed state after a second (with move & grow changes I
    found myself wanting to shift modifier keys without releasing them,
    this means I can just pause)
  • refactor code to make it easier (for me) to understand
  • tweak the documentation a little

Major changes:
- add `move` and `grow` methods to move and grow (or shrink) windows
- fullscreen starts by centering the window, then proceeds through the
  sequence
Minor changes:
- update hs.grid when obj.GRID changes, making obj:init() unnecessary
- forget obj._pressed state after a second (with move & grow changes I
  found myself wanting to shift modifier keys without releasing them,
  this means I can just pause)
- refactor code to make it easier (for me) to understand
- tweak the documentation a little
@miromannino
Copy link
Owner

Hello Matthew! Thank you for the updates.
There is one major problem I am noticing by quickly trying it:

  • If you try to open for example a Finder window that have a random size (e.g. try to resize it with the mouse a little). Clicking hyper+F it doesn't resize to full screen, but the window is just moved on the center (even keep pressing hyper+F nothing happens anymore for that window).

  • I just quickly skim the code, but I noticed some things that are probably not going to be easy to understand (e.g. _grid_store_yeeV5hiG, _grid_value_store_Oocaeyim).

Thank you!

@matthewfallshaw
Copy link
Author

Yes, that looks like a bug. Sorry about that. I'll review and update the pull request in the next few days.
(… and I'll add some more comments around the _grid_store metatables.)

@matthewfallshaw
Copy link
Author

Updated, with some extra comments.
(That huge block of code around _grid_store_yeeV5hiG can all be removed if you prefer restoring obj:start() and requiring users to remember to call it after changing GRID.)

@matthewfallshaw
Copy link
Author

Hold on - I just found another bug.
The 'c' option I've added to fullScreenSizes doesn't work if the window starts at one of the sizes in your fullScreenSizes array (say spoon.MiroWindowsManager.fullScreenSizes = {1, 'c', 4/3, 2} and your window starts maximised. You call spoon.MiroWindowsManager:goFullscreen() and the code detects the window in position 1 so advances it to position 2… which is the same. You call spoon.MiroWindowsManager:goFullscreen() again and it again detects that it's in position 1… so you can't ever get to position 3.)
I'll fix that in the next few days.

handle repeated sizes in sizes and fullscreenSizes, 'c' sizes when there is nothing stored, and some other edge cases.
@matthewfallshaw
Copy link
Author

Okay - I think that's good now. Let me know if you have any problems.

@miromannino
Copy link
Owner

miromannino commented Jul 4, 2018

Hello @matthewfallshaw! Thank you again!
I am not sure why but the full screen is still not working.
Sometimes the window goes to the center and resized a little, but then it stays there even if I keep pressing hyper+F. After days and days I figure it out that is only when the window is small, for example whenever I open a new terminal window.

Have you experienced this?

@matthewfallshaw
Copy link
Author

If I had to guess I would guess that it's the values you have in MiroWindowsManager.fullScreenSizes - that if you multiply each of the numbers in fullScreenSizes by the numbers in GRID you're getting non-integer values, which is causing rounding, which is causing something like, for example:

  1. You hit the fullscreen command, it matches you to, say the second of the sizes in fullScreenSizes so tries to move you to the third of them.
  2. That size is non-integer so hs.grid snaps to the nearest integer size, which happens to match the second again, so
  3. When you hit the command again exactly the same thing happens.

If you share your hs config I'll try to reproduce the problem you're having.
eg.

local mwm = hs.loadSpoon("MiroWindowsManager")
mwm.sizes = {2, 3/2, 3, 'c'}
mwm.fullScreenSizes = {1, 'c', 4/3, 2}
mwm.GRID = {w = 24, h = 12}

@miromannino
Copy link
Owner

Uhm that is possible, but I am using anyway the one in the code:

obj.sizes = { 2, 3, 3 / 2 }
obj.fullScreenSizes = { 1, 4 / 3, 2 }
obj.GRID = { w = 24, h = 24, margins = hs.geometry("0x0") }

@matthewfallshaw
Copy link
Author

I can't reproduce the problem you describe on either of my monitors. https://youtu.be/_GzGINYi1cg
So… erm… I'm not sure where to go from here. I guess I'll try to convince a couple of friends to try out my fork, and see if any of them have the problem you're describing.

Thank you, anyway, for the code. I was very pleased with your window manager as you originally built it, and I'm very happy with the way it works for me with my changes. Even if we can't get to the bottom of this and you never accept my pull request, I remain grateful for your code.

@miromannino
Copy link
Owner

Ok then I will try to see if I will have time to debug and see what is the problem.

It feels like something simple, but I didn't try to debug and see at all.

I would like to have this feature, but also the full screen to work. During the debug I will also try to replicate that in a video so you might better know the parts of the new code that could be problematic.

@miromannino
Copy link
Owner

Hey Matt thank you so much for this! It looks so cool!

I have been trying this for a couple of days before merging.

I feel like everything is good, but I am not a big fan of the moving actions. Don't you think would be nicer to keep moving the windows on keydown instead of on key press? If I need to move the window a lot I need to keep pressing that arrow many times. Also I noticed that I move left for few times and then I move right (without releasing ctrl+cmd) and the window resizes fill width!

Also (but this is minimal) I have conflicts with other hotkeys (that usually people use for changing workspaces), I need to check if someone can do something like ctrl+cmd+alt+X and then using arrows. Do you know if that is possible?

@matthewfallshaw
Copy link
Author

Don't you think would be nicer to keep moving the windows on keydown instead of on key press?

Yes, absolutely. This is just a straightforwardly good point and an obvious improvement.

Also I noticed that I move left for few times and then I move right (without releasing ctrl+cmd) and the window resizes fill width!

I actually fixed this problem a few days ago, then didn't like the repetition of having to call cancel_press on each hs.hotkeys.bind call, so started looking for a more elegant way to do that, then ran out of time and forgot to push the change. Fix pushed.

Also (but this is minimal) I have conflicts with other hotkeys (that usually people use for changing workspaces), I need to check if someone can do something like ctrl+cmd+alt+X and then using arrows. Do you know if that is possible?

Yes, easy to do with hs.hotkey.modal, and trying that out has been on my list for a long time (I loved the elegance of your initial keyboard mapping, and didn't like that I was complexifying it with my new features).
The code is easy to write and I think the only thing stopping me has been sitting down to work out what keyboard mappings to use. Do you want to propose the keyboard mapping you want and (if I like that) that's what I'll implement?

@miromannino
Copy link
Owner

Hey Matt! I did some experiments yesterday night changing your new code a little and I have few ideas. Let me work on it during the weekend. I think the best would be, let me try to integrate your changes on a v1.2 branch and we work on it for a while until we are both happy with the new interaction and features.

@rjhilgefort
Copy link

Appreciate the discussion and work on this PR! I love this project and love to see some iteration on it! Just wanted to let you guys know that I just set myself up on @matthewfallshaw's fork and configured hotkeys this way:

local hyper = {"ctrl", "alt", "cmd"}
local hyperShift = {"ctrl", "alt", "cmd", "shift"}

hs.loadSpoon("MiroWindowsManager")

hs.window.animationDuration = 0.1
spoon.MiroWindowsManager:bindHotkeys({
    up = {hyper, "k"},
    right = {hyper, "l"},
    down = {hyper, "j"},
    left = {hyper, "h"},
    fullscreen = {hyper, "return"},
    moveUp = {hyperShift, "k"},
    moveDown = {hyperShift, "j"},
    moveLeft = {hyperShift, "h"},
    moveRight = {hyperShift, "l"}
})

I think the "move" stuff is great for big monitors and allows me to more easily create some custom layouts that don't force me into corners. That said, I don't think I fully understand why the "grow" stuff is needed. Perhaps I'm just being blind to the value those commands provide? I also am a bit worried by the default binds conflicting with other apps, but people like me can just map things like I've done.

I'm going to play around with this for a little bit while you guys finalize things and I'll let you know if I find any bugs or issues. Let me know if there's anything you want to see if I can reproduce on my machine or anything like that. I don't know Lua all that well, but I'm happy help test and what not.

Thanks again for all the effort!

@miromannino
Copy link
Owner

For the moving feature: would you prefer modal (e.g. hyper+v and then using arrows to move and esc or any other letter to exit from that modal) or having that you need to keep pressed hyper+v and use arrows to move?

I don't like the idea of having to use letters such as k,j,h,l. It's the same thing as other apps that needed to use letters to move windows to corners. It's basically why this project exists.

@matthewfallshaw I am reviewing the code, but, why saving the GRID is actually needed? Is there any situation where people change the GRID size and they don't reload hammerspoon all over?

@miromannino miromannino changed the base branch from master to v1.2 October 26, 2018 07:30
@miromannino miromannino merged commit 431c632 into miromannino:v1.2 Oct 26, 2018
@rjhilgefort
Copy link

For the moving feature: would you prefer modal (e.g. hyper+v and then using arrows to move and esc or any other letter to exit from that modal) or having that you need to keep pressed hyper+v and use arrows to move?

🤔 That could be nice, yeah. I don't currently do anything modal with my window management keys, but if it was configurable, I would give it a shot. In general, I'm for "layers" and modal interfaces.

I don't like the idea of having to use letters such as k,j,h,l. It's the same thing as other apps that needed to use letters to move windows to corners. It's basically why this project exists.

As a Vim user, h, j, k, l are my arrow keys. In fact, I don't even have arrow keys on my keyboard! I'm running a 40% board called Planck.

img_20140903_182825

Thanks to both of you for the coordination on this addition and the work in general! I like the changes so far!

@miromannino
Copy link
Owner

Yes I also have similar setup. So I'll go with hyper+v and keeping it pressed you can use arrows (or letters) to move the window.

@miromannino
Copy link
Owner

Hello guys, I don't know if you get notified from updates, but I pushed things on the weekend on the 1.2 branch.

You can configure with something like:

spoon.MiroWindowsManager:bindHotkeys({
  up          = {hyper, "up"},
  down        = {hyper, "down"},
  left        = {hyper, "left"},
  right       = {hyper, "right"},
  fullscreen  = {hyper, "f"},
  center      = {hyper, "c"},
  move        = {hyper, "v"},
  resize      = {hyper, "d"}
})

For moving you need to keep hyper+v pressed and use the arrows to move the window. Arrows or whatever letters depends on how up,down,left, and right were configured.

Same for resize, keep pressed hyper+d and use the arrow: up = taller, down = shorter, right = wider, left = thinner. Same as what @matthewfallshaw proposed. But here again arrow or letters is decided depending on the up,down,left, and right actions.

I wanted to be like this because I just want a hyper and arrows (or WASD, or HJKL) for all, and you can do modifier of that by pressing an extra letter. This avoid confusions and I am already pretty confident by using it.

I also fixed the 'c' option on the fullscreen that sometimes wasn't working. I put it as main interaction: obj.fullScreenSizes = {1, 2, 'c'}. So first time fullscreen, second time 1/2 of the screen size, third time it goes back to whatever size and position it was before.

Sorry @matthewfallshaw if I changed half of the things you wrote and removed a lot, but I feel like I needed to keep it more minimal for expanding and fixing bugs.

Please tell me what you think about the new code and if you like it!

@rjhilgefort
Copy link

@miromannino Thanks for the update! I'll set up my computer with your recent changes some time this week and try it out!

@rjhilgefort
Copy link

rjhilgefort commented Nov 10, 2018

I wanted to be like this because I just want a hyper and arrows (or WASD, or HJKL) for all, and you can do modifier of that by pressing an extra letter. This avoid confusions and I am already pretty confident by using it.

Yeah, I really love this configuration scheme and interface! I have been playing around with it and it feels great! 👏 👏 👏

@miromannino I think I'm seeing a bug though. I have my configuration as the following and my "hyper + l" (right) doesn't do anything. To try to debug a little bit, I put a different key for right and it worked again. I think perhaps something about the new code on the v1.2 branch is binding something to l and it's preventing my config from binding to it? To further clarify, hyper + l was working just before I updated to the new code.

local hyper = {"ctrl", "alt", "cmd"}

hs.loadSpoon("MiroWindowsManager")

hs.window.animationDuration = 0.1
spoon.MiroWindowsManager:bindHotkeys({
    up         = {hyper, "k"},
    right      = {hyper, "l"},
    down       = {hyper, "j"},
    left       = {hyper, "h"},
    fullscreen = {hyper, "return"},
    center     = {hyper, "c"},
    move       = {hyper, "v"},
    resize     = {hyper, "d"}
})

@matthewfallshaw
Copy link
Author

@miromannino This is great. Thank you again. I've suggested some edits and made another pull request over at #9

@rjhilgefort Yes, Miro had what looks like a few lines of debugging code left in there that would have overwritten your {"ctrl", "alt", "cmd"} + l binding (some logger.i lines that would have been invisible to you unless you had run spoon.MiroWindowsManager._logger.setLogLevel('info')). I've boldly suggested removing them in my new pull request.

@rjhilgefort
Copy link

Awesome @matthewfallshaw, thanks for following up! I'll follow your PR for the fix then.

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.

3 participants