feat: UI work to facilitate future improvements in midi-into-track-input #183

qm210 · 2024-11-22T12:50:34Z

To make #182 easier, these are the changes that are only refactorings or UI improvements

(I dared to sneak in one change, namely the tracker now opens in full screen because the 800x600 is really annoying me, but if you do not want that in here, I might try to do some early work on #180 or just keep that change in my local repo)

qm210 · 2024-11-22T12:51:04Z

tracker/gioui/buttons.go

-	if b.Bool.Value() {
+	if !b.Bool.Enabled() {
+		ret.Color = disabledTextColor
+		ret.Background = transparent
+	} else if b.Bool.Value() {


make Enabled() state visible on ToggleButtons

qm210 · 2024-11-22T12:51:27Z

tracker/gioui/buttons.go

+		if b.Hidden {
+			return layout.Dimensions{}
+		}


Buttons can be hidden (space is a precious resource)

Instead of Hidden, why not just skip calling the Layout altogether? For widgets that need to handle input despite being hidden, the better solution is to split the Layout and Update calls, with Update taking care of all input handling and calling Layout can be skipped if there's no need to draw the button.

qm210 · 2024-11-22T12:52:32Z

tracker/gioui/label.go

-	return LabelStyle{Text: str, Color: color, ShadeColor: black, Font: labelDefaultFont, FontSize: labelDefaultFontSize, Alignment: layout.W, Shaper: shaper}.Layout
+	return SizedLabel(str, color, shaper, labelDefaultFontSize)
+}
+
+func SizedLabel(str string, color color.NRGBA, shaper *text.Shaper, fontSize unit.Sp) layout.Widget {
+	return LabelStyle{
+		Text:       str,
+		Color:      color,
+		ShadeColor: black,
+		Font:       labelDefaultFont,
+		FontSize:   fontSize,
+		Alignment:  layout.W,
+		Shaper:     shaper,
+	}.Layout


hm. not sure about this anymore, I added a specific need for the SizedLabel at one point, but I am not sure (it might be in the upcoming midi input PR though)

qm210 · 2024-11-22T12:53:52Z

tracker/gioui/menu.go

+						// note: might be a bug in gioui, but for iconColor = mediumEmphasisTextColor
+						// this does not render the icon at all. other colors seem to work fine.
+						iconColor = disabledTextColor
 						textLabel.Color = mediumEmphasisTextColor


this was weird behaviour: other than the code suggested, if iconColor was set to mediumEmphasisTextColor, it really displayed no icon at all if not Allowed().

with my upcoming menu (for midi vel track selection), I really want the possibility to show an icon on a disabled menu item. this change makes it possible.

qm210 · 2024-11-22T12:54:10Z

tracker/gioui/menu.go

-								p := gtx.Dp(unit.Dp(m.IconSize))
+								p := gtx.Dp(m.IconSize)


IDE told me this conversion was a noop

IDE is right :) I also noticed that the language server started to make this kinds of suggestions (didn't do that earlier), just didn't have time to clean everything up yet.

qm210 · 2024-11-22T12:54:47Z

tracker/gioui/menu.go

+								if icon == nil {
+									return D{Size: gtx.Constraints.Min}
+								}


also, popup Menu Items had to have an icon (IconBytes) for now, now these are optional

qm210 · 2024-11-22T12:55:49Z

tracker/gioui/note_editor.go

-		in := layout.UniformInset(unit.Dp(1))
-		voiceUpDown := func(gtx C) D {
-			numStyle := NumericUpDown(t.Theme, te.TrackVoices, "Number of voices for this track")
-			return in.Layout(gtx, numStyle.Layout)
-		}
+		voiceUpDown := NumericUpDownPadded(t.Theme, te.TrackVoices, "Number of voices for this track", 1)


this refactoring was because I first thought I want to add another NumericUpDown here. I changed my mind about this, but this change still makes the code less complex to read, I believe, so I kept it in

qm210 · 2024-11-22T12:57:18Z

tracker/gioui/note_editor.go

+func (te *NoteEditor) paintColumnCell(gtx C, x int, t *Tracker, c color.NRGBA, ignoreEffect bool) {
 	cw := gtx.Constraints.Min.X
 	cx := 0
-	if t.Model.Notes().Effect(x) {
+	if t.Model.Notes().Effect(x) && !ignoreEffect {


when paintColumnCell is called to highlight a cell background inanother track, it is useful to ignore that cell Effect value or one can only color the current Nibble() - usually not intended. (actually, only intended for the current cursor)

qm210 · 2024-11-22T12:57:57Z

tracker/gioui/numericupdown.go

@@ -74,11 +80,22 @@ func NumericUpDown(th *material.Theme, number *NumberInput, tooltip string) Nume
 		Tooltip:         Tooltip(th, tooltip),
 		Width:           unit.Dp(70),
 		Height:          unit.Dp(20),
+		Padding:         unit.Dp(padding),
 		shaper:          *th.Shaper,
 	}
 }


I think I do not specifically use the NumericUpDownPadded anymore (see above, I changed my mind), question is whether I should remove the change

Padding is still used, but I removed the wrapper calls to the constructor

qm210 · 2024-11-22T12:59:03Z

tracker/gioui/songpanel.go

-	MenuBar        []widget.Clickable
+	MenuBar        []Clickable


all the widget.Clickable -> Clickable and widget.Button -> Button changes are extensions of our custom buttons (to make them ignore key presses). These are not thoroughly consistent, but I will migrate them wherever I need them.

qm210 · 2024-11-22T13:00:01Z

tracker/gioui/theme.go

+
+func withScaledAlpha(c color.NRGBA, factor float32) color.NRGBA {
+	A := factor * float32(c.A)
+	return color.NRGBA{
+		R: c.R,
+		G: c.G,
+		B: c.B,
+		A: uint8(A),
+	}
+}


unclear whether such a helper function should live in this specific file, but I found that every existing color-morphing-function inside gioui package was not exported (or I did not look correctly)

yeah, they were uneager to export them. Apparently, there's better color handling functions in "out there" (according to them), so they don't want to maintain public API for the color functions. But I don't know what that "out there" could be; probably meant some library (or is there something in the standard library)? Anyway, I didn't look into this more deeply.

qm210 · 2024-11-22T13:00:21Z

tracker/gioui/tracker.go

-					w = new(app.Window)
-					w.Option(app.Title("Sointu Tracker"))
-					w.Option(app.Size(unit.Dp(800), unit.Dp(600)))
+					w = NewWindow()


this is only a de-duplication here...

qm210 · 2024-11-22T13:00:54Z

tracker/gioui/tracker.go

+func NewWindow() *app.Window {
+	w := new(app.Window)
+	w.Option(app.Title("Sointu Tracker"))
+	w.Option(
+		app.Size(unit.Dp(800), unit.Dp(600)),
+		app.Fullscreen.Option(),
+	)
+	return w
+}
+


...but here, I added the Fullscreen by default. is there a reason against it?

hm. I reverted this change and pushed, am not sure why github doesn't update

qm210 · 2024-11-22T13:01:13Z

tracker/gioui/unit_editor.go

@@ -332,7 +332,6 @@ func (p ParameterStyle) Layout(gtx C) D {
 				gtx.Constraints.Min.Y = gtx.Dp(unit.Dp(40))
 				instrItems := make([]MenuItem, p.tracker.Instruments().Count())
 				for i := range instrItems {
-					i := i


It's not a lol, it's a bug fix. But you are right, Golang 1.22 fixed this, so it's not needed anymore. See: https://go.dev/blog/loopvar-preview

oh. really?? sorry, that just looked so far beyond anything having real use, but I'll be the first to admit having way too little knowledge about golang or its history...

Yeah, it's a bit silly, that's why golang people changed the language semantics. Interestingly, same mistake in language design was made by C#, see https://learn.microsoft.com/en-us/archive/blogs/ericlippert/closing-over-the-loop-variable-considered-harmful. And they also changed the semantics of the language so you don't have to do var v2 = v type of assignments.

But so far, this has been the only breaking change in v1 of go; disregarding this, all old go code since v1 still compiles & should work.

Why language designers make this mistake? My guess is that it's because at early stages of their language design, their compiler is not very good at optimizing, and var v2 = v or i := i (which actually is exactly identical i.e. defining a new variable, the name just overlaps the old name) both are NOT nops, but do some unnecessary memory / register assignments. Just a guess.

qm210 · 2024-11-22T13:07:35Z

tracker/optional_int.go

we sometimes have (or will have) integers parameters that are optional, and I do not like using values like "-1" for "parameter is not set". thus, I borrow from the Optional structure that other languages offer

So the question is: what integer parameters exactly will be these Optional ones?

for the input-MIDI-into-the-tracks feature, I want the possibility to input MIDI velocity into a given track, that needs to be specified by the user beforehand. The Track Index is a integer. This feature should be optional (having no velocity input at all, midi input loses a significant part of its dynamic, but maybe the user is fine with that).

As I'm used to optional constructs from other languages, I tend to implement such types when needed, but if this is too much "you ain't gonna need it" for now, I can carry a tuple for now

vsariola · 2024-11-22T12:58:15Z

tracker/gioui/buttons.go

+		if b.Hidden {
+			return layout.Dimensions{}
+		}


Instead of Hidden, why not just skip calling the Layout altogether? For widgets that need to handle input despite being hidden, the better solution is to split the Layout and Update calls, with Update taking care of all input handling and calling Layout can be skipped if there's no need to draw the button.

vsariola · 2024-11-22T13:03:01Z

tracker/gioui/numericupdown.go

+	return NumericUpDownPadded(th, number, tooltip, 0)
+}
+
+func NumericUpDownPadded(th *material.Theme, number *NumberInput, tooltip string, padding int) NumericUpDownStyle {


I think making many versions of a NumericUpdown* that return slightly different NumericUpDownStyle:s smells a bit to me. Can you just not do:

s := NumericUpDown(...) s.Padding = (what you want)

In the case you need to change the default padding is not suitable? Or will there be so many places where we need to have something else than the default padding?

About Hidden: the idea originated from note_editor.go layoutButtons, which is strucutred as

return Surface{Gray: 37, Focus: te.scrollTable.Focused() || te.scrollTable.ChildFocused(), FitSize: true}.Layout(gtx, func(gtx C) D { addSemitoneBtnStyle := ActionButton(gtx, t.Theme, te.AddSemitoneBtn, "+1") subtractSemitoneBtnStyle := ActionButton(gtx, t.Theme, te.SubtractSemitoneBtn, "-1") addOctaveBtnStyle := ActionButton(gtx, t.Theme, te.AddOctaveBtn, "+12") subtractOctaveBtnStyle := ActionButton(gtx, t.Theme, te.SubtractOctaveBtn, "-12") noteOffBtnStyle := ActionButton(gtx, t.Theme, te.NoteOffBtn, "Note Off") deleteTrackBtnStyle := ActionIcon(gtx, t.Theme, te.DeleteTrackBtn, icons.ActionDelete, te.deleteTrackHint) splitTrackBtnStyle := ActionIcon(gtx, t.Theme, te.SplitTrackBtn, icons.CommunicationCallSplit, te.splitTrackHint) newTrackBtnStyle := ActionIcon(gtx, t.Theme, te.NewTrackBtn, icons.ContentAdd, te.addTrackHint) voiceUpDown := NumericUpDownPadded(t.Theme, te.TrackVoices, "Number of voices for this track", 1) effectBtnStyle := ToggleButton(gtx, t.Theme, te.EffectBtn, "Hex") uniqueBtnStyle := ToggleIcon(gtx, t.Theme, te.UniqueBtn, icons.ToggleStarBorder, icons.ToggleStar, te.uniqueOffTip, te.uniqueOnTip) midiInBtnStyle := ToggleButton(gtx, t.Theme, te.TrackMidiInBtn, "MIDI") return layout.Flex{Axis: layout.Horizontal, Alignment: layout.Middle}.Layout(gtx, layout.Rigid(func(gtx C) D { return layout.Dimensions{Size: image.Pt(gtx.Dp(unit.Dp(12)), 0)} }), layout.Rigid(addSemitoneBtnStyle.Layout), layout.Rigid(subtractSemitoneBtnStyle.Layout), layout.Rigid(addOctaveBtnStyle.Layout), layout.Rigid(subtractOctaveBtnStyle.Layout), layout.Rigid(noteOffBtnStyle.Layout), layout.Rigid(effectBtnStyle.Layout), layout.Rigid(uniqueBtnStyle.Layout), layout.Rigid(Label(" Voices:", white, t.Theme.Shaper)), layout.Rigid(voiceUpDown.Layout), layout.Rigid(splitTrackBtnStyle.Layout), layout.Flexed(1, func(gtx C) D { return layout.Dimensions{Size: gtx.Constraints.Min} }), layout.Rigid(midiInBtnStyle.Layout), layout.Flexed(1, func(gtx C) D { return layout.Dimensions{Size: gtx.Constraints.Min} }), layout.Rigid(deleteTrackBtnStyle.Layout), layout.Rigid(newTrackBtnStyle.Layout)) })

and I thought a way to set midiInBtnStyle.Hidden would make as clear as code can get.

I agree that the same purpose can be fulfilled by what I found out
midiInBtnStyle = layout.Spacer{}

but setting midiInBtnStyle to EmptyWidget() wouldn't work here because the layout.Rigid(midiInBtnStyle.Layout) below still expects something that has a Layout to call.

due to missing ternary operators, I found it a bit cumbersome / didn't want too much case distinctions inside the layouting itself, but I see that the Hidden has the disadvantage that one would have to implement it for every widget again, and this is a lot of duplication.

another spontaneous idea could be:
layout.Rigid(hiddenIf(condition, midiInBtnStyle.Layout)),

with

func hiddenIf(condition bool, widget layout.Widget) layout.Widget { if condition { return layout.Spacer{} } return widget }

but I'm free for any new idea

I pushed a new version with a similar proposal, and doing so, I already included the actual hiding of the button.

this branch has the problem, that calling RTMIDIContext.InputDevices() always iterates through all available devices. The upcoming branch has a fix by caching the number of devices after looping through them once. if you want, I can also get that improvement into the current branch.

vsariola · 2024-11-22T13:05:26Z

tracker/gioui/numericupdown.go

 		shaper:          *th.Shaper,
 	}
 }

 func (s *NumericUpDownStyle) Layout(gtx C) D {
+	if s.Hidden {


again, I'm wondering if we need Hidden, or can just skip calling Layout altogether? You can make "empty" or "null" widget just by making a function something like:

func EmptyWidget(gtx C) D { return D{} }

that you can pass to any function that expects a widget and you don't want one.

yes, see above

vsariola · 2024-11-22T13:16:21Z

tracker/gioui/tracker.go

+	w.Option(app.Title("Sointu Tracker"))
+	w.Option(
+		app.Size(unit.Dp(800), unit.Dp(600)),
+		app.Fullscreen.Option(),


Well, I don't like the fullscreen default, sorry :( Starting full screen by default is rude to me, considering the user can hit ctrl-enter to go fullscreen. No app desktop app does that, except games, and especially with the VST, there will be other windows around on the screen.

If you insist on this, please make the change of saving the window resolution and recalling that. Although tbh I don't know if there's any way to ask gio if the window is fullscreen or not; if not, then we may have to start making a user configuration file, where the user can define if fullscreen is wanted or not.

Well, for me, assuming a too limiting resolution is more rude than starting with a maximized window. But ok, I think the config file would be a good compromise (which can later be extended for the solution discussed in #180)

I certainly don't want to push my favourite workflow onto others, but I am too easily disturbed by simple nuisances (not proud of this, it's just my brain), and either we have a way where users can config that preferences (a .yml file or similar is fine), or I would stay with using my fork (which would be a pity when others could benefit from such changes) 🫤

I think user configuration file is really the only practical way, or recalling some settings like they were last when the window was closed. Or even some combination. We've already seen this with keyboard shortcuts that people have their own tastes, and whatever we now define as the default behaviour, there's always someone who'd prefer it to be otherwise :) So, let's just make configuration file, I think that's actually pretty easy. Let me open an issue for it.

vsariola · 2024-11-22T13:26:04Z

tracker/optional_int.go

I think you need to explain a bit why this is needed, and why we cannot just e.g. interpret value -1 to have special meaning on the GUI side? What's the planned use case of OptionalInt and OptionalInteger?

I guess I commented on that already, but to make sure:

if I allow an index to have the value -1 for some special purpose, I would have to check at every possible call, e.g. Tracks[optionalTrackIndex] whether that trackIndex is -1. plus, I would feel responsible to make clear, possible in some comments, what the -1 means because I can in no way expect that everybody knows what the -1 means. This is the disadvantage of magic values - why not write code that speaks for itself and has less dangerous side effects?

vsariola · 2024-11-22T15:18:55Z

tracker/gioui/note_editor.go

 			layout.Rigid(splitTrackBtnStyle.Layout),
 			layout.Flexed(1, func(gtx C) D { return layout.Dimensions{Size: gtx.Constraints.Min} }),
-			layout.Rigid(midiInBtnStyle.Layout),
+			layout.Rigid(OnlyIf(t.HasAnyMidiInput(), midiInBtnStyle.Layout)),


Question: what's wrong with

layout.Rigid(func (gtx C) D { if t.HasAnyMidiInput() { return midiInBtnStyle.Layout(gtx) } return D{} })

If this place is the only reason to introduce EmptyWidget and OnlyIf, the total number of lines is far less, and it's immediately obvious what's going on here.

vsariola · 2024-11-22T15:33:42Z

tracker/types/optional.go

If you really want to do Optional type, should we use generics? i.e.

type Option[T any] struct { value T exists bool }

There is a bit of convention that New* functions return pointers & do a heap allocation, and Make* functions return value types (structs) to construct a value. But I would much prefer calling them something shorter e.g. Some[T any](value T) and None[T any]() (or Empty as you did). I think golang is then clever enough that you don't need the T when it can be inferred so if a is int, Some(a) will return Option[int]. I'm pretty sure it needs the type for None[int]() though.

would not oppose that idea,

is there, by any chance, something similar for the tracker/optional_int.go parts?

I mean, it's maybe not clear from this pull request, but in the other one, this was intended to be used as
https://github.com/vsariola/sointu/pull/182/files#diff-eabff715d9f08388ede1907be0dbf2f5c9526e82944c0a9e76ecfad80ac4ead3R48

I'm not sure. Maybe not worth the effort to try to make Int so generic that it would work both with Optional<int> and int as its underlying data.

feat: UI work to facilitate future improvements in midi-into-track-input

ad690c7

qm210 commented Nov 22, 2024

View reviewed changes

qm210 mentioned this pull request Nov 22, 2024

feat: input midi velocity into a separate track #182

Draft

vsariola reviewed Nov 22, 2024

View reviewed changes

qm210 force-pushed the midi/prepare-midi-input-improvement branch from a1787f3 to 417bcdc Compare November 22, 2024 14:27

fix: from review

243396c

qm210 force-pushed the midi/prepare-midi-input-improvement branch from 417bcdc to 243396c Compare November 22, 2024 14:46

vsariola reviewed Nov 22, 2024

View reviewed changes

feat: UI work to facilitate future improvements in midi-into-track-input #183

Are you sure you want to change the base?

feat: UI work to facilitate future improvements in midi-into-track-input #183

Conversation

qm210 commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qm210 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qm210 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qm210 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

vsariola Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

vsariola Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

qm210 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

vsariola Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

qm210 Nov 22, 2024 •

edited

Loading

qm210 Nov 22, 2024 •

edited

Loading

qm210 Nov 22, 2024 •

edited

Loading

vsariola Nov 22, 2024 •

edited

Loading

vsariola Nov 22, 2024 •

edited

Loading

qm210 Nov 22, 2024 •

edited

Loading

vsariola Nov 22, 2024 •

edited

Loading