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

Keeps the 1/2->1/3->2/3 window behavior, but resets the cycle anytime... #258

Merged
merged 4 commits into from
Feb 7, 2018

Conversation

kjrokos
Copy link
Contributor

@kjrokos kjrokos commented Dec 13, 2017

… a window is moved outside of the cycle

Copy link
Collaborator

@rca rca left a comment

Choose a reason for hiding this comment

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

The behavior is much better!

Can you clean up the trailing whitespace being added to the files, please.

@rca
Copy link
Collaborator

rca commented Dec 13, 2017

@rca
Copy link
Collaborator

rca commented Dec 13, 2017

Awesome, thank you!

Now, question is, what is the final desired behavior. This is already better than the existing release, but some folks would like this feature disabled altogether.

I am not familiar at all with the code and don't know how easy or hard it is to add a toggle-able preference.

@kjrokos
Copy link
Contributor Author

kjrokos commented Dec 13, 2017

No problem, glad I could help! I'm not really familiar with the entire codebase either*; the change above I made was pretty limited in scope -- I'm willing to take a look, but may take some time to familiarize myself with what I need to know. That said, it should be fairly simple, code wise, to add a toggle. To do some of the other things people were requesting (user-defined split values, ignoring the second move command and then changing the split on the third, etc) would require a more serious refactoring of the code.

* or with Objective-c in general - my professional experience is C++ and Java

@fikovnik
Copy link
Owner

Amazing!

Regarding the settings, I think there are two ways how to implement it:

  1. a quick one in which anyone who wants to disable it will have to run a command like defaults write org.shiftitapp....halvesOnly
  2. add a GUI for it in the preference window - basically

The first option involves:

@rca
Copy link
Collaborator

rca commented Dec 16, 2017

@kjrokos please comment on whether you think you'll have time to implement @fikovnik's suggestion above. Thank you.

@kjrokos
Copy link
Contributor Author

kjrokos commented Dec 16, 2017

Yes, I can get you a PR in the next few days

@kjrokos
Copy link
Contributor Author

kjrokos commented Dec 17, 2017

With the latest commit, you can now disable the functionality by using

defaults write org.shiftitapp.ShiftIt multipleActionsCycleWindowSizes NO

Copy link
Collaborator

@rca rca left a comment

Choose a reason for hiding this comment

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

This is great, thank you. I am not sure if you're familiar with git add --patch, which helps me skip things I don't intend to commit.

<key>nextscreenKeyCode</key>
<integer>45</integer>
<key>nextscreenModifiers</key>
<integer>1835008</integer>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an intentional whitespace change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More or less. I used Xcode's plist editor, and it automatically made the whitespace changes; but had I edited it manually I would have likely done the same thing to make the file more consistent.

@rca
Copy link
Collaborator

rca commented Dec 17, 2017

Did you, by chance, make cycling default to enabled or disabled? Given the backlash, perhaps it should be opt-in?

Also, adding the command in your comment above to the README is ideal.

I'm going to put together a new build ...

@rca
Copy link
Collaborator

rca commented Dec 17, 2017

Ok, just tried this myself and to enable cycling I had to run this command:

defaults write org.shiftitapp.ShiftIt multipleActionsCycleWindowSizes YES

I think opt-in is good!

@rca
Copy link
Collaborator

rca commented Dec 17, 2017

Adding reference to #254

@kjrokos
Copy link
Contributor Author

kjrokos commented Dec 17, 2017

Yeah, I wasn't sure what the default should be. I think most of the backlash was because of the bug where going back and forth between directions changed the size (that was certainly the impetus for me getting involved), rather than the functionality itself.

I think for new installs, the default behavior will be for it to be enabled, because they don't have the plist installed yet, and I have the default set to enabled. For upgrades, the behavior will be for it to be disabled, because the key doesn't exist in their already existing plist.

@kjrokos
Copy link
Contributor Author

kjrokos commented Dec 17, 2017

(I didn't test the new install, though)

@rca
Copy link
Collaborator

rca commented Dec 17, 2017

That sounds reasonable.

If you don't mind updating the README, I think this is good to merge into master. @fikovnik thoughts?

@jackjennings
Copy link

jackjennings commented Jan 22, 2018

Can this get merged in soon? The new behavior that this pull request corrects bothers me every time I use the application…

@rca rca merged commit 3d671b9 into fikovnik:master Feb 7, 2018
@rca rca added this to the 1.6.5 milestone Feb 7, 2018
@rca rca changed the title Keeps the 1/2->1/3->2/3 window behavior, but resets the cycle anytime… Keeps the 1/2->1/3->2/3 window behavior, but resets the cycle anytime... Feb 7, 2018
@jackjennings
Copy link

Thanks so much @rca

cjcjameson added a commit to cjcjameson/workstation-setup that referenced this pull request Feb 12, 2018
`brew cask install shiftit` is getting the latest from Github at the current
time (v1.6.5) which fixes fikovnik/ShiftIt#258
@nmccready
Copy link

With the latest commit, you can now disable the functionality by using

defaults write org.shiftitapp.ShiftIt multipleActionsCycleWindowSizes NO

Was this made the default (NO)? I only ask as the resizing used to work without the need to run.
defaults write org.shiftitapp.ShiftIt multipleActionsCycleWindowSizes YES

This is why I ended up on this thread.

@MattRyder
Copy link

Hey! You broke my workflow!

I found the 'reduction cycle' feature recently, and thought this was default/intended behaviour.
I was about to file a bug report, glad the defaults functionality exists. All working on v1.6.6. 👍

@rca
Copy link
Collaborator

rca commented Mar 5, 2018

Per discussions I believe the intent was to maintain the old behavior and if someone wanted the additional 1/3 window in the flow the defaults command was required.

If this is not the case, please reply back and we should discuss maintaining old behavior as the default.

professor pushed a commit to pivotal/workstation-setup that referenced this pull request Mar 13, 2018
`brew cask install shiftit` is getting the latest from Github at the current
time (v1.6.5) which fixes fikovnik/ShiftIt#258
@concretevitamin
Copy link

As someone with auto update on, upgrading to 1.6.6 broke this behavior and I had to google, found this thread, and run defaults write org.shiftitapp.ShiftIt multipleActionsCycleWindowSizes YES. I believe important -- and useful -- behaviors like this one should be kept backwards-compatible. At least, make them visible in the GUI settings.

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.

7 participants