Add instrument switcher buttons to InstrumentTrackWindow #1987

Merged
merged 1 commit into from May 2, 2015

Projects

None yet

8 participants

@Wallacoloo
Member

This implements the feature described in #1937, or at least how I interpreted the request.

lmms-instrument-switcher

It treats instruments inside the B/B editor as a separate group from those inside the song editor, so if you're iterating through B/B instruments, it skips over the ones in the main song editor and vice-versa. Additionally, it does work with multiple instrument windows. If clicking the next/prev arrow would cause the InstrumentTrackWindow to be editing another instrument already open, then it simply closes the other editor.

There are currently no shortcuts for activating the next/previous arrows (which I believe the original feature request included), but it would be trivial to add if they can be agreed upon.

@tresf
Member
tresf commented Apr 20, 2015

This is very exciting. @badosu @curlymorphic I see no issues at a glance. This is a fairly large impacting feature (file-wise) considering how close to a freeze we are, but Colin is on a roll... :)

Anyway... A second set of eyes would be great prior to merging.

@Wallacoloo
Member

:-)
I keep seeing references to an upcoming freeze, but I can't find any detailed information on it (dates, etc). Is there some roadmap somewhere (the link in the wiki is dead)?

@curlymorphic
Contributor

nice work @Wallacoloo , as @tresf said, you are on a roll :)

I keep seeing references to an upcoming freeze, but I can't find any detailed information on it (dates, etc).

This was announced on the mailing list https://lists.sourceforge.net/lists/listinfo/lmms-devel

Im still newish here, but from what I can see there is no defined roadmap as to when releases happen
it seems to be "when we have enough done to call it a release", we do have https://github.com/LMMS/lmms/milestones/1.2.0 as a guide aswell, but I am assuming those labed as enhancements will be bumped to 1.3, leaving us the bugs to clean up. oh and any bugs found in testing.

Im am going to take a look at this pull request now. It is definitely a feature that will help clean up our ui.

If clicking the next/prev arrow would cause the InstrumentTrackWindow to be editing another instrument already open, then it simply closes the other editor.

My initial concern is if we can handle the above in a more elegant way. I will wait until I have had a look before i comment further, but I dont feel that should stop us having this in 1.2.

@curlymorphic
Contributor

@Wallacoloo you say

it does work with multiple instrument windows.

How do I now open a second instrument window?

@tresf
Member
tresf commented Apr 20, 2015

@Wallacoloo Dave's right, we don't have any observable milestones because -- until recently -- we didn't have the resources to collaborate on major features, so we're still operating from the bug-fix mentality, mixed with some maverick-coding; This means our features and bugs mix together in a never-ending bug tracker (hence the initiative to consolidate and merge the frequently requested or related items).

1.2

Here's a snapshot of our release notes for 1.2:

https://github.com/tresf/lmms/wiki/tmp

Note, these notes are not visible to non-admins because of how GitHub tracks draft releases.

2.0

2.0 has some milestones in it, (see LMMS Futuremap #908 bug report) but is is mostly lead by Vesa, whom has been mostly unavailable for a few months.

-Tres

@tresf
Member
tresf commented Apr 20, 2015

How do I now open a second instrument window?

If single instrument mode is enabled, disable it, then click the button on another instrument track. :)

@curlymorphic
Contributor

If single instrument mode is enabled, disable it, then click the button on another instrument track. :)

I feel silly now 👍

@curlymorphic
Contributor

I have tested this and all works, The code looks good aswell.

I am still concerned about just closing duplicate windows. But from looking at the code, I understand why this approach is taken, rather than having new instances of the ui's. I would find it annoying to use them buttons to find my other windows inadvertently closed. Maybe it would be an option to skip over already open instruments.

@Wallacoloo
Member

It's possible to just bring the other window to the front and give it
focus. That's probably the simplest solution.

Alternatively, we can show the arrows only in single window mode until we
come up with a better solution.
On Apr 20, 2015 10:06 AM, "Dave" notifications@github.com wrote:

I have tested this and all works, The code looks good aswell.

I am still concerned about just closing duplicate windows. But from
looking at the code, I understand why this approach is taken, rather than
having new instances of the ui's. I would find it annoying to use them
buttons to find my other windows inadvertently closed. Maybe it would be an
option to skip over already open instruments.


Reply to this email directly or view it on GitHub
#1987 (comment).

@Spekular
Contributor
@Sti2nd
Contributor
Sti2nd commented Apr 23, 2015

Oh, that was what unfa meant! I thought those arrows would open another preset ;)

@Wallacoloo
Member

Updated the behavior as per @Spekular and @curlymorphic .

If you find out you don't like that behavior, there is another solution as well:
Say instrument A and B both have windows 1 and 2 respectively, but instrument C is closed (and the instruments are ordered A, B, C on the track list). From window 1, if we click "next", we can replace window 2's contents with C, and replace window 1's contents with B. This way clicking next always replaces the window's content with the next instrument without closing any windows. The downside is that the content of your other windows does get shuffled.

@tresf
Member
tresf commented Apr 26, 2015

I'd like to embrace this for 1.2 but I'd like feedback from some others prior to merging.... Namely @Sti2nd @mikobuntu @Umcaruje.

@mikobuntu
Contributor

@tresf On initially just checking through the posts here I like this a lot. Question, :- in a large song with lots of BB_Editor track, if I am at the top of the list ( i.e) the 1st track ) and I use the back arrow will it scroll through the instruments from the bottom of the BB_Editor window... this would be a great time saving feature if so. Also is there a branch i can check out to test for myself... thanks :)

@mikobuntu
Contributor

@tresf cool, will report back on my thoughts ;)

@musikBear

@Spekular Thanks i only saw your tip by chance - i dont get a flag-up for '@' -or maybee i do, but dont know where to look :p
This new feature 👍 -Only thing is -What happens if you have to

  • 'browse' through really many presets (like from 1. to 13.)
  • browse past several VSTs presets (memory-leak?? And what if the VST GUI is open?

Has those been tested?
(Time for binaries ? 😸

@mikobuntu
Contributor

@tresf It's a yes from me, this is a really intuitive feature. @musikBear it seems to work ok for switching through instruments quickly, even while playing in midi notes. Once a VST(i) has loaded there is no difference in loading time either, remember this is just an instrument loader, presets are a totally different concept. EDIT I think we only need this behaviour in single window mode tho.

@mikobuntu
Contributor

@Wallacoloo On a slightly "off topic" note, what do you use to record your desktop as a .gif image or is this done as an after edit...thanks 💯

@Wallacoloo
Member

@mikobuntu that seems to be a popular question around here ;-) I use a program called byzanz-record.

So if I understand you right, you'd like it so that whenever you switch to a new instrument with the nav buttons, the BB-editor (if applicable) automatically scrolls so that the newly opened instrument's track is visible? For consistency, would the song viewer do the same thing?

@mikobuntu
Contributor

@Wallacoloo I have actually had that installed from a PPA before but somehow never noticed it could do that thanks.
Yes that would be an added bonus as we would quickly be able to see at a glance our midi data of the track and also have quick access to open in piano roll, without having to scroll down to see which track is active. Having the same behaviour across both Song_Editor and BB_Editor in my opinion is always a good thing for consistency :) EDIT byzanz is now in universe repos ...installed:)

@Sti2nd
Contributor
Sti2nd commented Apr 26, 2015

I'd like to embrace this for 1.2 but I'd like feedback from some others prior to merging

Not sure what kind of feedback you want, testing? I would recommend LMMS 1.3 since this is a new feature, but I guess that was not the kind of feedback you wanted from me...

@Wallacoloo
Member

@mikobuntu I'll try to add that behavior sometime later this week.

@Wallacoloo
Member

@mikobuntu & others: the BB-editor or Song-editor now auto-scroll to make sure that the newly switched-to instrument track is visible :-) The scrolling behavior isn't explicitly smoothed, but it looks fine to me that way. Should be possible to implement smooth scrolling if anyone requests it though.

@tresf
Member
tresf commented Apr 30, 2015

Thanks Colin.

It looks like there are some merge conflicts now. Once they are resolved and you think it's ready (probably get the OK from @mikobuntu), we'll merge. 👍

@mikobuntu
Contributor

@Wallacoloo good work, I'm not sure we even need a smooth scroll. So it's an OK from me, I will test when the merge conflicts are sorted ;)

@Wallacoloo
Member

Ok, I merged the changes from master & all is working well on my end. Let me know if I goofed up somewhere, as I'm still fairly new to git.

@tresf
Member
tresf commented Apr 30, 2015

@Wallacoloo that's quite the long chain of commits. I would recommend squashing them into a single commit. It requires you to re-write history and force push the changes, but it will make the commit history much cleaner.

@Wallacoloo
Member

@tresf good thinking. I couldn't squash everything into one commit without getting a bunch of errors, so I reduced it down to 1 commit for the changes + 1 commit for resolving the merge conflict. Hope that's ok!

@tresf
Member
tresf commented May 1, 2015

Looks pretty good. Ideally, the upstream changes shouldn't appear in the commit history.

I'm sure you have better things to do than hack at git all day, so if you get fed up with it, we can merge as-is.

@Wallacoloo
Member

@tresf Alright, I got it down to one commit - should be ready for testing @mikobuntu.

@tresf
Member
tresf commented May 2, 2015

Nice work. Thanks for all the hard work as well as tackling the Git trouble. Merging, as we can easily tweak it for 1.2 RC.

In terms of nomenclature, we should probably decide on a unidirectional naming convention moving forward.

I would almost advise we just keep fooMoveNext, and make fooMovePrevious call fooMoveNext(-1); (rather than fooMoveIn()) but we can refactor that at a later time. 👍

@tresf tresf merged commit 9f95c04 into LMMS:master May 2, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Reaper10

This could all so be useful fx plugins two.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment