-
Notifications
You must be signed in to change notification settings - Fork 16
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
Multi-Unit View #181
base: master
Are you sure you want to change the base?
Multi-Unit View #181
Conversation
I have been travelling and didn't have time to look into this. I hope to have time next weekend. |
I added the lines to CHANGELOG.md for the past commits that were forgotten. But the rest of this change is a bit intimidating with so many changes :D I'll try to read carefully what is going on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I like the idea, the UI can be pretty confusing now. For example:
The texts indicate that "Outaux" is selected but then there is Envelope unit there shown. Also the fact that the units are vertical in the instrument list and the horizontal in the multiunit view bothers me quite a bit, but I can see why you did it like that.
Do we really need to have both the multiunit view and the single unit view?
Rather than having several different GUI modes, I'd prefer the multiunit mode to be so intuitive that it can be the only mode and we can save some code and some user confusion by not having multiple different modes.
Did you consider the split view similar to VSCode? There's this button that splits the view in the top right corner of the VSCode:
The disadvantage is that there would be more space wasted on the unit list if it's repeated every view. But the advantage is that it would be far less confusing as you see exactly which unit you are on the unit list, like it is now. The implementation would be something like that we have struct e.g. like InstrumentView
type InstrumentView {
InstrIndex, InstrIndex2 int
UnitIndex, UnitIndex2 int
ParamIndex int
UnitSearchIndex int
UnitSearchString string
UnitSearching bool
}
and the Model would have slice with 1 or more InstrumentViews. Then on the gioui side generate a new InstrumentEditor for each InstrumentView. Just a thought, not sure how massive rewrite this would be.
Then a couple of code related things:
-
Callbacks are not nice. Instead, the gioui / imgui way is that the widgets should indicate that something has happened (like Clickable.Clicked()), which is called by whoever is calling the Layout, and there we can take appropriate action in response to things having changed.
Related: we really should refactor so that Layout and Update methods are separate from each other for each widget / editor. Update should do all input handling and deciding how to respond to those, and Layout would do only the layout. Update can also return (event, ok) where events are things that might be interesting to the higher level. For example, draglist could somehow signal that selected item was changed etc. I think this refactoring can be later, but the Layout methods are becoming beasts. -
Left and right arrow keys don't seem to change the parameters anymore.
-
The colors indicating what is selected is a bit weird, e.g.:
What is selected here?
@@ -168,6 +168,9 @@ type Clickable struct { | |||
history []widget.Press | |||
|
|||
requestClicks int | |||
|
|||
// optional callback for custom interactions | |||
OnClick func() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not comfortable with adding callback for Clickable; the imgui way of doing this should be for whoever called Layout to check for Clicks with something like:
for widget.Clicked() {
doSomething();
}
...
widget.Layout(...)
```
I am thinking of refactoring also away the fixed binding of buttons to actions, but I have not found the way to do that yet.
@@ -31,6 +31,7 @@ type DragList struct { | |||
swapped bool | |||
focused bool | |||
requestFocus bool | |||
onSelect func(index int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly, I don't like callbacks for widgets when something happens. Instead, we could have something like:
for event,ok := dragList.Update(gtx); ok; event,ok = dragList.Update(gtx.) {
// draglist.Update can return different kinds of events to tell what has happened, like Clickable.Update returns clicks
}
...
dragList.Layout(...)
@@ -105,6 +105,11 @@ func NewInstrumentEditor(model *tracker.Model) *InstrumentEditor { | |||
ret.linkDisabledHint = makeHint("Instrument-Track\nlinking disabled", "\n(%s)", "LinkInstrTrackToggle") | |||
ret.linkEnabledHint = makeHint("Instrument-Track\nlinking enabled", "\n(%s)", "LinkInstrTrackToggle") | |||
ret.splitInstrumentHint = makeHint("Split instrument", " (%s)", "SplitInstrument") | |||
ret.unitDragList.onSelect = func(index int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes away when getting rid of the callback and making it index,ok := dragList.Selected()
@@ -386,7 +392,7 @@ func (ie *InstrumentEditor) layoutUnitList(gtx C, t *Tracker) D { | |||
if ie.searchEditor.requestFocus { | |||
// for now, only the searchEditor has its requestFocus flag | |||
ie.searchEditor.requestFocus = false | |||
gtx.Execute(key.FocusCmd{Tag: &ie.searchEditor.Editor}) | |||
gtx.Execute(key.FocusCmd{Tag: ie.searchEditor.Editor}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look correct to me. It's the address of the widget that is always its tag by convention. ie is a pointer, searchEditor is a pointer, but Editor is a field, so we should take its address. Are you sure about this?
} | ||
ret.caser = cases.Title(language.English) | ||
ret.copyHint = makeHint("Copy unit", " (%s)", "Copy") | ||
ret.disableUnitHint = makeHint("Disable unit", " (%s)", "UnitDisabledToggle") | ||
ret.enableUnitHint = makeHint("Enable unit", " (%s)", "UnitDisabledToggle") | ||
ret.multiUnitsHint = "Toggle Multi-Unit View" | ||
|
||
ret.MultiUnitsBtn.Clickable.OnClick = func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No callbacks here either. Instead when updating the widget, check Clickable.Clicked() and take some action if it happened.
Alternative idea: we put all the units in a vertical list, but we flow sliders so that if two sliders fit on the same line we put them horizontally. This way all the units are always vertically, both in the left list and in the right list. The list could go on even past an instrument, so you can scroll up and down to see all the parameters of a unit. Oh, and we would save a lot of space by using knobs instead of sliders, maybe that way it would not be so awkward to have all the knobs of a unit horizontally? |
I love the idea with using knobs instead of sliders! |
I think there's a certain "rack modules" vibe with having knobs horizontally and units stacked vertically |
I always found this kind of presentation very appealing - https://www.beat.de/media/beat/styles/tec_frontend_large/public/images/2020/06/10/image-76056--30523.png |
Maybe we could even take the rack metaphor further and draw lines to indicate how units connect signals to each other. |
Finally, for people with ultrawide monitors, ability to split view would stillbe useful. But the split view would essentially duplicate the entire instrument editor. |
Maybe make a "horizontal rack" switch for the ultrawidescreen ppl? :) |
I think it'll be almost trivial to draw line rails on the left hand side, with the line shifted to the right equal to its position in the stack so they don't overlap. Of course, works too if all units are horizontal. But having them flowing so that sometimes they are horizontal and sometimes vertical is a problem for drawing those lines. |
Let's move the discussion back to #173 because I think we're diverging from what this pull request is. |
(originally posted in #173 but it better belongs here) Hey, I like these ideas in general (or indeed, everything that goes towards "more visibility of what is going on" - especially on smaller screens), but am not sure, what, from this discussions, are the real blockers for the pull request and what are new issues for future refinement :) could we clarify? |
So, to clarify: I would prefer not merging this. To me, this is taking the GUI to a wrong direction, as it add complexity to code and confusion to user, without entirely solving the needs. Specifically:
The approach in this PR does not solve b, and only solves a in the limited case that the units are in the same instrument and quite close to each other in the unit list. But quite often, even within an instrument, you have all the synthesis done first, finishing with outaux, and then there's envelope->send(pop) combos in the end to make some modulations. These sends are already several units away from the modulated unit, so probably don't fit on the same view. Maybe we could merge this PR as a "temporary fix" and later change everything according to the whatever plan gets agreed on in #173, but I'd prefer we skip temporary solutions and try to go directly towards whatever we think will be ultimately the best. |
first implementation of #173
didn't test it that thoroughly yet.
there is at least one problem I couldn't solve:
the column widths get updated on the fly (because I need to have rendered every parameter widget at least once in order to know how wide they are), and thus, when the Button (bottom right) is first clicked after startup, the view can not scroll to the correct x position.
The width calculations also lead to some short "flickering" when each unit is rendered the first time, I didn't see an easy way around that
also, no idea how to make that tooltip stay inside the window.
PS: I also sneaked in the changelog edits from my last pull request, that I forgot - if you prefer these in an extra PR, I can do so ;)