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 segmented control style #6

Merged
merged 53 commits into from Apr 3, 2019
Merged

Add segmented control style #6

merged 53 commits into from Apr 3, 2019

Conversation

DivineDominion
Copy link
Collaborator

@DivineDominion DivineDominion commented Nov 10, 2018

Closes #2

I struggled a while with different approaches until I threw out NSTabViewController out of the window and began managing the toolbar in a custom controller.

Now you can configure the style upon initialization and have it your way. Replicating the transition animation was a bit tricky because the user could click faster than the transition, ending in an invalid view state; I added a PausableWindow that swallows user events to prevent this from happening without disabling the toolbar controls.

Enhancements I'd prefer to tackle in separate issues/PRs:

  • make the segmented control configurable: isCentered/alignment, isSizedUniformly (all segments have the same with; that's the current state)
  • make the toolbar version configurable: isCentered/alignment, toolbarStyle (icon, label, icon and label)
  • hide window management of PreferencesWindowController: make PreferencesWindowController internal, expose PreferencesController as a plain Swift object with a simple API
  • make preferences controllers configurable after initialization (which also makes it possible again to use init(fromCoder:))

Changes to the PR:

  • Extract PausableWindow into its own file and add a comment about what exactly it does and its use-case/purpose
  • Use own identifiers: extension NSToolbarItem.Identifier
  • consider rename crossfadeTransitions to animate
  • rename preferencesWindowController.showWindow(withPreference: .advanced)
  • add preferencesWindowController.change(to: .preferenceAdvanced) (obsolete)
  • change style from tabs to segmented controls on the fly
  • left-align toolbar items
  • center window around segmented controls

@DivineDominion
Copy link
Collaborator Author

Something I'd like to have feedback on: using Auto Layout to pin the content views to the window size; because on the one hand, it makes window size management trivial, but on the other you end up with somewhat unexpected constraint animations when the window size change is animated.

@DivineDominion
Copy link
Collaborator Author

@sindresorhus Did you have a chance to take a look? You want me to adjust something?

@sindresorhus
Copy link
Owner

I really appreciate the PR. I’ve just been too busy in real life to look at it yet (wedding and family visits). I’ll hopefully have a chance to look at it the coming week :)

@DivineDominion
Copy link
Collaborator Author

DivineDominion commented Dec 10, 2018

The latest commit adds a representation for what we may call "overflow menu", which is how AppKit displays segmented controls in toolbars that outgrow the window.

Example with a few duplicates of the 1st example app's preference to make the control too big:

image

In addition, we should also prevent this from happening by keeping a minimum window size, I would say. But nevertheless, if users force the window size down, this is a better fallback than the previous behavior, i.e. displaying an empty menu.

@DivineDominion
Copy link
Collaborator Author

@sindresorhus Did you have a chance to assess the code so far? :)

@DivineDominion
Copy link
Collaborator Author

Today's changes make prefs work with Auto Layout properly.

  • disable translatesAutoResizingMAskIntoConstraints in the preferences views in IB
  • add sufficient constraints to define the preferences views' size
  • use NSView.fittingSize to compute the new window frame during animations, effectively fixing when you save IB files at a size that won't fit the runtime size
  • use built-in NSViewController.transition(from:to:options:completionHandler:) for animations
  • cache and remove constraints when embedding the child view controller, fixing jiggling in transitions

@sindresorhus
Copy link
Owner

This is looking really promising. I finally have a chance to look through the code now.

@sindresorhus
Copy link
Owner

sindresorhus commented Jan 16, 2019

One thing I noticed is that when using the segmented control style and I change a tab, it should animate from the segmented buttons, meaning, the segmented buttons should not move.

@sindresorhus sindresorhus changed the title add Segmented Control Add segmented control style Jan 16, 2019
@sindresorhus
Copy link
Owner

I think it could be useful to have a public method to change tabs while the preferences window is open.

Something like:

preferencesWindowController.change(to: .preferenceAdvanced)


func applicationWillFinishLaunching(_ notification: Notification) {
window.orderOut(self)
}

func applicationDidFinishLaunching(_ notification: Notification) {
preferencesWindowController.showWindow()
preferencesWindowController.showWindow(toolbarItemIdentifier: .preferenceAdvanced)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this would read better as:

preferencesWindowController.showWindow(withPreference: .advanced)

Or something similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since showWindow would do the same as change(to:), what do you think about the latest change to showPreference? I hope it brings across that it opens the window if needed, too.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that's good.

@sindresorhus
Copy link
Owner

@DivineDominion Hey, still interested in finishing this? You are so close :)

@DivineDominion
Copy link
Collaborator Author

I can have a look at this with one of my next app's iterations. I find it surprisingly hard to animate both the window frame and the content size smoothly, so growing the window symmetrically left and right to make the segmented controls stick could take more time than I can spare in the upcoming week or two.

I'll schedule time for this, though. Thanks for the review so far :) Good to know it's only minor things.

Smaller icons would work, but look odd, so I think we should leave them
disabled completely for now
Cache all tab sizes first when the views are new and use these sizes.
Segmented controls at the top seem to affect the window frame size.
Now that it's not implicitly assumed anymore that all preferenceables
_are_ indeed view controllers, but could also be configuration objects,
the name should reflect this change.
Using NSTabBarViewController makes switching between the controls
hard: segmented controls are not supposed to be part of the toolbar.
@DivineDominion
Copy link
Collaborator Author

I actually didn't mean it had to be dynamically changeable. That doesn't really make sense outside the example app, so we don't really have to complicate the code for that. I was thinking just restarting the app when the button is pressed. Should be almost instantaneous anyway.

Gotcha. Changed it.

Also fixed the remaining issues. Centering the window is jiggling the segmented control around a bit, probably due to rounding errors. I tried floor and ceil in PreferencesTabViewController.setWindowFrame(for:animated:), but that didn't help either.

@sindresorhus
Copy link
Owner

From the HIG:

Restore the last viewed preference pane. If the user switches preference panes, your app should remember this change and show the same pane immediately the next time the user opens your preferences window.

We don't currently do that. I think we should do it when users call .show() without an argument, and mention in the docs that that should be the preferred way to show the preferences window, and refer to the HIG docs. This would be another benefit of having #14 and we could enforce it there.

@DivineDominion
Copy link
Collaborator Author

@sindresorhus Will happily add that, but again favor doing that in another PR to keep this focused on the segmented controls. Would that work with you?

@sindresorhus
Copy link
Owner

Yup, agreed. This PR is already very large. Moved it to a new issue: #16

@sindresorhus
Copy link
Owner

Can you update the readme to reflect the changes?

@sindresorhus
Copy link
Owner

Also fixed the remaining issues. Centering the window is jiggling the segmented control around a bit, probably due to rounding errors. I tried floor and ceil in PreferencesTabViewController.setWindowFrame(for:animated:), but that didn't help either.

I don't really know a solution to that either. If you cannot find a solution, I'm fine with just deferring it to later and move it to a new issue.

@DivineDominion
Copy link
Collaborator Author

I think I caught everything in the README. I also added screenshots of the two styles, but oddly the README preview on my branch doesn't work. I hope it's just due to it being a fork and relative paths not working correctly, but can you double-check this when you merge?

@sindresorhus
Copy link
Owner

Didn’t we decide on .show(preferencePane: .general)? The word “preferences” in .showPreferences feels moot as it’s already part of the controller name and the parameter. We also need to mention that we recommend .show() without an argument.

@sindresorhus
Copy link
Owner

Otherwise, the changes look great! 👌

@DivineDominion
Copy link
Collaborator Author

You're right, this totally went under my radar. I added a docstring for now; when #16 is implemented, the recommendation can be expanded and will make more sense IMHO

@sindresorhus sindresorhus merged commit 3b62df8 into sindresorhus:master Apr 3, 2019
@sindresorhus
Copy link
Owner

This is absolutely amazing. I'm very happy about the end result. Thank you so much for working so hard on this, @DivineDominion. I really appreciate it. 🙌

Should we do a new release with what we have here now or do you intend to work on more things in the near future?

Also, would you be interested in joining the project as a maintainer? No commitment required. Just want to highlight your contribution in the readme.

readme.md Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I forgot to double-check the images before merge, but they are definitely corrupt. Would you mind submitting them again?

@DivineDominion
Copy link
Collaborator Author

I was transitioning all my macOS apps to this framework, so I have sufficient skin in the game to help you with this. If you can use a helping hand, I'd be happy to contribute!

I don't have a strong opinion about releases. In fact, I prefer more frequent releases over big releases that take a long time. Adding this is kind of a big thing, compared to the original code base, so why not! Then fix the open issues and localization problems.

@DivineDominion DivineDominion deleted the segmented-control branch April 3, 2019 15:35
@sindresorhus
Copy link
Owner

I was transitioning all my macOS apps to this framework, so I have sufficient skin in the game to help you with this. If you can use a helping hand, I'd be happy to contribute!

When you have done so, I would be happy to list your apps in the readme.

@sindresorhus
Copy link
Owner

I also replied to #6 (comment), in case it gets lost.

@DivineDominion
Copy link
Collaborator Author

DivineDominion commented Apr 5, 2019

Cool! Am currently using this for:

Am transitioning this one when we're done with the next release:

@sindresorhus
Copy link
Owner

Cool! Am currently using this for:

Can you do a PR to add them to the readme? Maybe a new section above the FAQ section.

dezinezync pushed a commit to dezinezync/Preferences that referenced this pull request Dec 17, 2022
dezinezync pushed a commit to dezinezync/Preferences that referenced this pull request Dec 17, 2022
dezinezync pushed a commit to dezinezync/Preferences that referenced this pull request Dec 17, 2022
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.

None yet

2 participants