Improve UX of Squit package chooser #346
Conversation
LinqLover
commented
Oct 26, 2021
LinqLover
commented
- Add support for multiselection (251a8e5)
- Add cancel button (6f3dcd1)
- Use theme colors for buttons (6f3dcd1)
|
PS: Sorry for pushing to this repository instead of my fork; this was an accident. |
| selectedUntrackedPackages add: aPackage. | ||
| selectedTrackedPackages remove: aPackage ifAbsent: []. | ||
| self changed: #selectionsTrackedAt:. | ||
| self changed: #selectionsUntrackedAt:. |
There was a problem hiding this comment.
It seems that the list view on the left does not scroll any of the added items into view. Thus you may have a selection not apparent to the user after you click the < button.
May rather be a problem of the Morph?
There was a problem hiding this comment.
This is particularly tricky because you would have to click on the added items to have them removed from the selection. (Unlike in Windows list widgets, for example, where a single click on an item would deselect other already selected items, this does not happen in the current multi selection list Morph.)
I see no easy way to clear the selection either.
There was a problem hiding this comment.
You are addressing two points here, right?
-
Scroll newly tracked/untracked package into view: Obviously, this can only be done for one package if they are not located together. I have implemented that via 97619a5 by single-selecting the new item (the Morphic vocabulary here is unfortunately overloaded: a multi-selection list has both a (single) "selection" (
#getIndex/#setIndexor#getSelected/#setSelected) and a "selection list" (#getSelectionList/#setSelectionList). Only the single selection is used to scroll an item into view). -
Selecting/unselecting items: To my knowledge, the common ToolBuilder pattern for allowing the user to control selections is to offer them a few "select all/select none/select inversion" options in the yellow-button menu of the list. If you deem this important, I can add such a feature. Interested?
There was a problem hiding this comment.
Scrolling into view works nicely now, thank you. Your mention of the list manipulation actions reminds me of the Test Runner, which in turn reminds me that I have already seen the "focus item among multiple selected items" there. You have to click it right if you want to use the "Browse" action to browse a particular test class from the list...
I do not think that we need the full set here. In particular, select all and select inversion would not do any good most of the time. Select none could be useful though.
What I noticed when trying this out again is that, despite the scroll into view feature, I tend to forget that I still have something selected outside of the view. For example, select 60Deprecated in the untracked packages, then remove a package from your project, 60Deprecated stays selected and is easily forgotten. Likewise you can forget it if you just scroll down to search for something else. Subsequently I am surprised when I use the ">" button.
In a way, it would be more intuitive if you could only select consecutive blocks of categories... and if you start another selection, the previous one would be cleared. Much like Windows list views behave, but that's Squeak widgets vs. Windows widgets gestures...
Not having that -- and honestly the dialog is so seldom used that it may not be worth all the attention -- should we add something simpler like an indicator how many packages are selected? Or a label below the left list that reads "Selected: 60Deprecated, Balloon, Gofer" (it could undergo the elimination of sub-packages already, or not)? Could be in another pull request, in order not to halt this one much longer.
There was a problem hiding this comment.
Or overwrite the previous untracked selection when packages get untracked. So that after something gets untracked, only those untracked packages are selected, not anything that was previously selected. It would not completely solve the forgotten selection problem if you untrack Etoys and WebClient simultaneously, but may still be batter than keeping the previous selection. You could also select nothing and just scroll the first of the removed into view.
What do you think is the most acceptable tradeoff between usability/predictability and simple implementation?
There was a problem hiding this comment.
Option 2 makes much more sense to me - it provides a simple direct-manipulation style for undoing any track/untrack button. Implemented via 587e6a4.
Regarding the list selection, I have added a simple "select none"/"select inversion"/"select all" menu to each package list in the dialog (d98d888). I think the second and the third option could make sense too indeed:
While most of the time, "select none" may be of the greatest interest, the other two options "select all" and "selection inversion" fulfill two purposes: Not only offer they familiarity to the user (see also the test runner or the change list), but also might they become useful if the user is interacting in a different environment than Smalltalk where the total number of packages could be much smaller.
How do you like it? :)
There was a problem hiding this comment.
Well, the "in another environment" example seems a bit far-fetched to me (and if the list of packages is short there is also less need for the menu items), but now that the code is already there I see no harm in it.
LinqLover
force-pushed
the
feat/package-chooser-ux
branch
from
2d02f7b to
97619a5
Compare
|
Unless absolutely necessary to implement the feature under review, please avoid merging the upstream branches into pull requests. It makes it harder to find all the changes that the branch introduces. |
This way, it is more consistent with the untracked packages, and it does less likely suggest that the order in which packages are tracked matters. (They are appended to the end of the working copy anyway.)
…ng selection R`esolves #346 (comment) (dangling items in selectedUntrackedPackages after tracking) and #346 (comment) (overlapping origins of selectedUntrackedPackages after untracking).
…ction While most of the time, "select none" may be of the greatest interest, the other two options "select all" and "selection inversion" fulfill two purposes: Not only offer they familiarity to the user (see also the test runner or the change list), but also might they become useful if the user is interacting in a different environment than Smalltalk where the total number of packages could be much smaller. Note that prior to Morphic-ct.2043 (trunk), pluggable lists' menus inside a dialog can only be invoked by keyboard-focusing the list and then pressing escape.
LinqLover
force-pushed
the
feat/package-chooser-ux
branch
from
6da76eb to
d98d888
Compare
Alright, great idea, will do it in the future! In this particular case, GitHub reports a merge conflict, though ... (Just curious: What tools make it harder for you to find all the changes from a branch? Unless you create a defect merge commit due to #53 or #199, I think GitHub will display the right diff anyway. Is Squot missing a similar capability? :)) |
LinqLover
force-pushed
the
feat/package-chooser-ux
branch
from
8beecd4 to
87708b2
Compare
That's life if it goes on on the target branch. :-) You could rebase the feature, or pretend to merge into upstream (not the other way around) and push that to the feature branch instead of to the upstream branch (the merge commit will have the upstream branch as its first parent, not as its second as in your merges—and the commit message will indicate the upstream branch as the target, obviously). Actually the command line instructions GitHub gives to merge manually if the source branch is on a fork produce about the same result.
Umm actually I gave the wrong reason. The real one is that the branch does no longer contain only its own changes and kind-of changes its base in the middle of its commit stream, which can make backporting harder. But actually backporting does not really happen in this repository. This is the influence from my job chiming in, where we do have to backport/rebase features every once in a while to supply older releases with service packs. Still, not merging the upstream at least keeps the history cleaner. |
|
Thanks for the explanation, and thanks for the review! 🎉 |
