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

Use hs.hotkey.modal for modals, a bugfix, and some code cleanup #9

Open
wants to merge 42 commits into
base: v1.2
Choose a base branch
from

Conversation

matthewfallshaw
Copy link

Bugfix:

  • don't force windows from higher or lefter screens onto the primary screen when resizing

Refactoring:

  • use hs.hotkey.modal for move and resize modals
  • some code & documentation cleanup

Change:

  • support the same resize to full-width/height behaviour when pressing opposite directions simultaneously in Move and Resize Modes

@miromannino ,
Thank you again for your stellar work on this Spoon. I like your keyboard interface for Move & Resize Modes much more than I liked the one I created in my last pull request.

If you don't like the addition of jumping to full width/height after tapping opposite directions in Move & Resize Modes, remove ; growFullyModals[move]:enter() from line 564, ; growFullyModals[mapR[resize]]:enter() from line 583, then remove lines 584, 580 and 565.

- titleCase, round, frontmost… to locals (we don't need to pollute the global namespace)
- some indentation and documentation fixes
- and some minor cleanup to satisfy the lauc linter (line length, trailing whitespace)
and retire watchers & etc. that are no longer needed.
Extend double tap behaviour to move & resize states.
…width/height

didn't quite register; first entering the modal, then performing the action seems
to fix this.
@miromannino
Copy link
Owner

Hello Matt! I checked this but with the modal the window can't be moved unless you repeatedly press the arrows. Also I noticed in your changed the window move and resize are bound to just arrows, but people use also WASD or HJKL as arrows.

I'll review changes and bugfix, but I don't think I can merge this as is

@matthewfallshaw
Copy link
Author

matthewfallshaw commented Nov 15, 2018

Those are both excellent points, and I think I've just fixed them both.
I can now tap or hold hjkl as my move keys for both move mode and resize mode.
Sorry for the oversight.

@rjhilgefort
Copy link

@matthewfallshaw Maybe I installed your branch incorrectly, but I'm not seeing the move and resize keys behaving. I'm holding hyper+v and then using my "arrow" keys, but it's behaving as if I'm not holding v at all- it just acts like I'm holding hyper and whatever arrow key and moves to that corner/side. I think it's more likely that I haven't correctly picked up your code, but I don't have time to troubleshoot at the moment. I'll play around next chance I get, but I just wanted to let you know in case it's not my error. Here's my config:

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"},
    center     = {hyper, "c"},
    move       = {hyper, "v"},
    resize     = {hyper, "d"}
})

@rjhilgefort
Copy link

... aaaaannndd I hadn't pulled since switching my submodule over to your fork. Sorry for the noise on this thread. I've tested your changes and they work great for me! This seems to solve the issue I was seeing in the previous PR!

@@ -201,6 +207,18 @@ function obj:resize(growth)

fr = fr:intersect(frontmostScreen():frame()) -- avoid sizing out of bounds

if self.stickySides then

Choose a reason for hiding this comment

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

ooooo, nice! I would love to see this get merged so I can get back on the main branch. cc: @miromannino

@rjhilgefort
Copy link

@miromannino I'm still on this PR/fork for the functionality in it. I can't speak to the Lua code, but the functionality would be a great addition. Please consider merging or at least helping @matthewfallshaw make changes that you would like to see. Thanks!

@miromannino
Copy link
Owner

Ok I'll review this soon , the last time there were bugs and I never had a chance to see @matthewfallshaw fixes, sorry for that! Yes I would like to merge if everything is stable. For now @rjhilgefort please tell me if you find any bug so we can address it before merging to main

@rjhilgefort
Copy link

@miromannino Thank you!! This Spoon you've created is one of my favorite utilities on my Mac and I greatly appreciate the time you spend on it. Sorry to be a bother!

I've been running this fork for something like 2 months now and I haven't seen anything to mention. I'll continue to keep an eye out.

@miromannino
Copy link
Owner

I know I actually cannot think to work without it. :)
Ok thank you for the feedback, I'll merge soon! Thank you again @matthewfallshaw for the new features and ideas!

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
handle repeated sizes in sizes and fullscreenSizes, 'c' sizes when there is nothing stored, and some other edge cases.
matthewfallshaw and others added 22 commits March 2, 2019 20:19
- titleCase, round, frontmost… to locals (we don't need to pollute the global namespace)
- some indentation and documentation fixes
- and some minor cleanup to satisfy the lauc linter (line length, trailing whitespace)
and retire watchers & etc. that are no longer needed.
Extend double tap behaviour to move & resize states.
…width/height

didn't quite register; first entering the modal, then performing the action seems
to fix this.
* 'master' of github.com:matthewfallshaw/miro-windows-manager: (26 commits)
  Sticky sides (optionally stick to a bound side when shrinking windows).
  HS docs are broken and can't do word wrapping. [sad face]
  minor: cleanup
  bugfix: support holding move keys to keep moving or growing
  bugfix: support non-arrow keys as moving keys
  bugfix: sometimes double tapping opposing directions to grow to full width/height didn't quite register; first entering the modal, then performing the action seems to fix this.
  bugfix: don't let our fullscreen originalPositionStore get garbagecollected.
  minor: cleanup, docs & logging
  Use `hs.hotkey.modal` for `← + →` and `↑ + ↓` double taps, and retire watchers & etc. that are no longer needed. Extend double tap behaviour to move & resize states.
  bugfix: don't force windows from higher or lefter screens onto the primary screen
  Use hs.hotkey.modal for modal move & resize
  housekeeping - titleCase, round, frontmost… to locals (we don't need to pollute the global namespace) - some indentation and documentation fixes - and some minor cleanup to satisfy the lauc linter (line length, trailing whitespace)
  fixes and refactoring
  resize
  fullscreen completed
  fullscreen fixes
  Update MiroWindowsManager.spoon/init.lua
  first cleanup and refactorings
  `cancel_press` as `releasedfn` (keyup) for moves (prevents `growFully` on direction change).
  Add Center method
  ...
@jacoscaz
Copy link

@miromannino this really seems worth merging in. Doing so would also provide something akin to what #15 is about, removing the need to merge that one.

@jacoscaz
Copy link

I'm currently on this branch and everything seems smooth as butter, at least from a cursory look.

@miromannino
Copy link
Owner

Perfect! I am glad to know! I will then definitely merge! I just need to review @matthewfallshaw code before submitting. Thanks again @matthewfallshaw again for changes!

@jacoscaz
Copy link

jacoscaz commented Apr 13, 2021

@miromannino here I am for my yearly "thank you" message for your window manager and my personal +1 for this PR (which I've been using for one year now without encountering any glitch or issue). Do you still plan on merging this?

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.

4 participants