feat: improves PreferencesPanel UX, partially implements #49 #76
Conversation
gingerr
changed the title
|
@lwouis Next iteration is committed/pushed and was quite fun. The sliders are now in but there is an UX thing missing for them. They need to show the currently selected value somehow:
Apple talks about tickMark labels in their UI guidelines but at the first look the NSSlider class does not offer anything for that. I will look into this next (probably tomorrow). As usual, I am Interested in your feedback and critique.
Commit message/changelog
|
|
Hey @gingerr! Thanks for the work, it looks so much better than the current UI!
|
|
@lwouis Thanks! Naming: Yeah, this and the others can also get greatly improved once we got tooltips or something as you said in the ticket goals. Padding: I spent hours and hours trying to understand positioning with different views and nested views and still am not sure if I got it right. The naming, the methods and the system is not obvious at all. I had trouble to control the first row or following rows padding and positioning (auto layout was messing with me). I’ve changed all views to be nested Stackviews (horizontal and vertical) since in the current layout grids are not needed but we could change it back to grids anytime. I wanted each row to implement its layout itself, to give more power to them. The view is now a vertical stack. Code review and This pull requests: it’s alright, leave this waiting until you find time, you don’t have to work on this everyday 🍻. I would like to implement the values visualization for the sliders somehow. But you can merge this at anytime you like. If this is merged when in finish the next iteration I will open another branch and pull request. 😅 Mariusz |
There was a problem hiding this comment.
Hey this is a first look at your PR. I think I would like to merge it only after you've found a way to add text marks on the sliders as otherwise I imagine it's not very user friendly.
Overall I could follow the code except 1 or 2 tricks like the invisible text. I gave some advice and opened some discussion on how to make the code tidier. I've had similar issues with the rest of the codebase, and thought I would share my experience, but even for me it's still very open discussion since I have no friend who's an expert at Cocoa I could ask for advice.
|
@lwouis thank you for your in-depth review, appreciate it! Ive made a few of the requested or proposed changes and will continue to work on that tomorrow evening. Nothing new is committed yet.
Fully agree, gonna implement something here.
Thanks for that, I appreciate your feedback and our discussions. At work we mainly use Java (Frontend & Backend) and TypeScript (Frontend) so we are in the same boat (learning experience) here. Btw. I was unhappy with the default macOS altTab experience and had on my todo list to look into alternatives but an issue was their closed source nature. I just can't install that on my company MacBook without hesitation or approval (security, privacy, trust). Then Ive read about your application in an article which was presented one day in my google news feed (I had googled for all the alternatives 😄 weeks or months ago). Thanks for this project/application! |
|
@lwouis Still working on this. Made good progress yesterday evening. My hope is to have it ready today evening (EU time). |
gingerr
force-pushed
the
feature/preferences-panel-ux
branch
from
4c688f0
to
6b1e2e8
Compare
|
@lwouis Alright, the next iteration for the partial implementation of #49 is rebased and force pushed. Ive done all the changes we talked about, refactored some more and kept at implementing improvements and little features into this. Every time I thought its good enough there was something else popping up (bad UX or a different approach to solve the problem at hand). The obvious UX improvements are my attempt at a simple instant validation with visualisation for the TextFields, error handling via an sheet (modal alike), visual representation of slider values and the Preferences being live instead of application restart. Tell me what you think. Cheers Changelog from commit message
|
|
Hey, sorry for the delay! Here is some feedback from an e2e perspective:
I tested and these 2 issues pre-date your PR, so I will open a separate ticket for them. Other things:
I will review the code now Update: I finished reviewing. I asked for change instead of just merging and putting the comments as informational-only. The main reasons are:
|
| private func challengeNextInvalidEditableTextField() { | ||
| let invalidFields = (contentView? | ||
| .findNestedViews(subclassOf: PreferencesPanelNSTextFieldEditable.self) | ||
| .filter({ !$0.isValid() }) | ||
| ) | ||
| let focusedField = invalidFields?.filter({ $0.currentEditor() != nil }).first | ||
| let fieldToNotify = focusedField ?? invalidFields?.first | ||
| fieldToNotify?.delegate?.controlTextDidChange?(Notification(name: NSControl.textDidChangeNotification, object: fieldToNotify)) | ||
|
|
||
| if fieldToNotify != focusedField { | ||
| makeFirstResponder(fieldToNotify) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ok I understand now what you do. You block the user from leaving the window if a field is invalid. I think you made it way harder for yourself!
- Easiest/strongest fix: remove these annoying text fields and put sliders. Done. User can't make mistakes now
- Intermediate fix: keep the previous logic to reset the field as soon as an incorrect value is written and focus on that field is gone. This guarantees the field has a correct value always.
- Too-intense fix: the approach you took here where you want to communicate more to the user but I believe you end up confusing them more than previous 2 approaches as the red field doesn't explain what's wrong next to it, and when I close the panel, it says it's wrong but not why, or which format is expected. Also what if I don't close the window but close the app, or simply switch the window to the background and expect my change to be active? I think you're going into a territory that would require integrating a full-blown validation framework, but in our case it's way simpler to do the solutions above.
There was a problem hiding this comment.
Gotcha. Removed the text fields for the other fields as mentioned in another code review comment. Tab key input is the only one remaining for the moment.
Thats focus/unfocus is my problem. I have issues to get notified when the input looses focus anyhow (panel closed, dropdown opened, another input selected) to trigger a save Preference operation. If we don't trigger the save the last somehow here the value the user entered will not be saved at all. The panel before this pull-request has the same issue. If the user reopened the panel, he saw whatever he entered, even if it was not saved or the Preferences rejected the value. The PreferencesPanel did only save if the user focused an other input. This is my attempt at fixing this without requiring the user to click something.
One of the workarounds I came up with was the invisibleTextField (to trigger an unfocus of the current input which then triggered a save). Now this invisibleTextField workaround is removed and the inputs are triggering a save on every key press. But we need to allow the inputs to be invalid while the user is editing them (or has the panel not closed yet), that means to skip execution of an save (or there will be an exception triggering the error sheet panel).
One earlier iteration I had it that way, a user was not able to leave an input while it was not valid but that felt not right. Since there is now only one input text field remaining I could try to find out how I did it and reimplement it.
Once we have some way of recording a key or key combination (#68) we could remove all the TextField handling/complications.
dosn't explain what's wrong next to it, and when I close the panel, it says it's wrong but not why, or which format is expected.
Fair enough.
Also what if I don't close the window but close the app, or simply switch the window to the background and expect my change to be active?
I handled that 😬 The field validates instantly and gives feedback in time. The Preference will be saved to Preferences and disk if its valid. If the user close forces the window or the application and reopens, he will see his last valid value for the said Preference.
I think you're going into a territory that would require integrating a full-blown validation framework, but in our case it's way simpler to do the solutions above.
Yes, once the textfields are gone completely this will be a happy little code to look at.
| var whitelistedKeycodes: [Int] = Array(0...53) | ||
| whitelistedKeycodes.append(contentsOf: [65, 67, 69, 75, 76, 78, ]) | ||
| whitelistedKeycodes.append(contentsOf: Array(81...89)) | ||
| whitelistedKeycodes.append(contentsOf: [91, 92, 115, 116, 117, 119, 121]) | ||
| whitelistedKeycodes.append(contentsOf: Array(123...126)) | ||
| return whitelistedKeycodes.contains(int) | ||
| } | ||
| let maxThumbnailsPerRowValidator: ((String)->Bool) = { (3...16).contains(Int($0) ?? -1) } | ||
| let windowDisplayDelayValidator: ((String)->Bool) = { (0..<10000).contains(Int($0) ?? -1) } |
There was a problem hiding this comment.
See, you go really detailed on the validation here, but you don't surface that to the user. I think my comment above about just putting sliders is the way to go. For the tab key, I think for now let's let the user type an incorrect int for now. We are gonna replace this soon with a lib, so no need to spend time here I think
There was a problem hiding this comment.
You are right, the special nature of this input is just hinted KeyCode (48 = Tab) by its suffix wording.
This is just an improvement pull request for the related issue and I required some kind of fast and simple validation when I added that red visualisation state to the other textfields: an invalid key code would be a valid CGFloat (so the panel would not throw any error for that). This keycodes array corrects that and is an attempt on an easy fix/improvement to the current situation.
I planned to link that nice keycodes url as a 'good enough' solution via a button or clickable text (in the grey suffix textfield) in an upcoming iteration of this PreferencesPanel since its planned to integrate/implement some keycode recording in the future.
I think Sliders and PopUps for everything are also the way to go here and TextFields only where it makes absolute sense to use one. They overcomplicate this Panel and open a can of worms with validation & visualisation.
- ux: first pass on UI - ux: groups available preferences by keys-, visual-, behaviour-preferences (vertical separation without labels) - ux: adds Restart action - code: removes unused variables - code: PreferencesPanel is no longer a Delegate of an view child
- ux/code: adds sliders (maxScreenUsage, iconSize, fontHeight) - ux: defined sensible min/max/step values for sliders - code: fixes panelWidth (panel and its content did not grow correctly) - code: removes inputsMap (NSControl.identifier is now used for that) - code: adds makeLabelWithProvidedControl() as a helper to create labeled controls with a optional suffix - code: removes dropdownDidChange() & textDidEndEditing() -> combined in controlDidEndEditing() - code: adds controlDidEndEditing() as an universal .action for NSControl elements - adds a restore of values in UI and Preferences if there is an issue while saving a Preference - code: adds another little code clarity improvements
implements requested changes from Code-Review and contains also refactoring and more features and improvements - Application Wide: changes all exceptions to NSError (required for error handling via sheet in panel) - PreferencesPanel: adds display of current value of Sliders - PreferencesPanel: adds validation with visualisation for TextFields (whitelist for KeyCode and ranges for maxScreenUsage & windowDisplayDelay) during editing, window open and close - PreferencesPanel: adds error sheet (in-window modal) at all known possible errors to display error information and ask the user how to proceed - PreferencesPanel: removes Restart button (and relaunch code in Application) - PreferencesPanel: adds Preferences being now live - PreferencesPanel: removes invisibleText workaround - PreferencesPanel: adds TextField value save on each keyDown event (if valid and changed) - PreferencesPanel: adds window activation/deactivation - PreferencesPanel: changes label texts of some Preferences - ...
implements requested changes from Code-Review and contains also refactoring, additions & improvements - PreferencesPanel: does not handle NSApp.activate & .deactivate anymore - PreferencesPanel: maxThumbnailsPerRow & windowDisplayDelay are now sliders - PreferencesPanel: Sliders do not use tickmarks anymore (to allow finer settings) - PreferencesPanel: tweaked Sliders min/max values - PreferencesPanel: adds Hyperlink to KeyCodes Reference as suffix for Tab key control (with additional required sub-class of NSTextField) - PreferencesPanel: adds makeSuffix() method for code clarity
gingerr
force-pushed
the
feature/preferences-panel-ux
branch
from
6b1e2e8
to
6b395a0
Compare
No problem at all! Thanks for reviewing the pull-request in detail once again!
Yeah, coming from web development I was wondering about that one too but it looks like its the normal native behaviour so I stopped there:
I can't reproduce that. If I repeat that steps the 4 will be valid for me. Anyway, this Preference is a slider now, as you suggested. I was wondering the same and had on my todo list to ask you, which preferences should become sliders (all of them?) and which min/max values you would choose for them.
Corrected. Had a issue with tickmarks here: having too much or to little of them and the slider not starting at 0 for this Preference and me picking the wrong tickmarks divider. I made the all the sliders without tickmarks now. Another divider for the amount of tickmarks would be also possible. My motivation for the tickmarks was, to prevent the hammering of Preferences.saveToDisk() since its get called pretty often with continuous sliders. I had them non-continuous before, but the current slider value visualisation implementation requires them to be continuous again. A debounce (as in rxjs observables) would be a better solution here I think. 256 sounds also interesting! I think we should also allow 0 for
Done. The infinite value would require a different non-llinear scale (however this is done with NSSlider) or could be simply solved by an additional checkbox that disabled the slider?
Thanks for the detailed review! Yes it does, sorry for that. I think my unfamiliarly with Cocoa does also add to that. Ive done the requested changes and additional little improvements, see other comments. |
|
I tested the PR locally and I think it is in great shape! Thanks you for all the work on this! By the way if you have time to hang out in the Discord, I would like to discuss next tickets :) |
|
Nice! Thanks for the merge 🤝 and making my adventure into macOS ui lands possible. |
Hey @lwouis
This is my first improvement iteration of the PreferencesPanel. My target was to improve the visuals. This is ready for review and integration into upstream/master if you like.
Mariusz
Issue reference: #49