Rosegarden is great, but 2-3 seemingly minor UI things really waste a lot of my time and nerves when I try to work on larger projects. This is one of them:
Sorry if I missed something, but by default Rosegarden seems to just forward into long silence after playing a track, with no obvious simple way to change it. This seems like a huge anti-feature to me, if I keep the playback going I want to keep the song going to continue previewing and editing it. (Otherwise I would have pressed stop...?) Sadly, setting a loop range over a longer song is very tedious because when zoomed out to cover the entirety of it, it is almost impossible to set it accurately to the proper start/end of first and last measure of the song. Also, it is a huge time waster to readjust it every time I added another measure and want to preview the entirety of it. Even worse, I need to redo this manually every time I selected a short section temporarily for looping and I want to go back to the entirety of the song.
As a result, I think the current loop behavior is almost unusable, and I'm cursing every time I need to touch it.
This is my proposed fix: I think a reasonable behavior would be instead to break up the IMHO nonsensical link of the transport loop toggle to the loop range. I should be able to toggle looping without a loop range, and it should NOT reset me to the old possibly partial loop range. Instead, if no loop range is set it should loop back immediately after the last non-empty item to the first measure. Setting a loop range may optionally still enable the transport loop button, or not, I have no strong feelings on this, but clearing the range should definitely not unset it. This way I could set partial loop ranges as I wanted, and when clearing out go back to looping the entire song without wasting an entire minute trying to fiddle the giant whole song loop range back into place. (This is also how REAPER, a commercial DAW, handles this by the way - "do a loop button" and "loop range slider" UI decoupled.)
Tested with Rosegarden 20.12.
Thanks.
Yeah, I wasn't sure about it either. I need to do some testing to get a better feel for how it's working now. Just got done writing up a test plan for the previous behavior (without loop all) which is different depending on whether the new loop range is before (jumps) or after (no jump) the previous loop range. That's given me lots of ideas for how this might work in a performance setting. Anyway, we should get the basics working then worry about that later.
Ok, I've put together a test plan for the classic looping behavior which we must preserve when "Loop entire song if no range is set” is disabled in the preferences. The results with the changes for this issue are not good. The transport buttons no longer work. I gave up after the first few tests.
The test plan is here:
https://www.rosegardenmusic.com/wiki/test:classic_looping
Please run it and see what you can do to get this all working again. I'm ok with a few minor improvements/deviations here and there (like the improved right-click behavior). But the original behavior must be preserved when "Loop entire song if no range is set” is disabled in the preferences. I suspect it needs to be more of an "Advanced looping" preference that switches between the original behavior and the new.
I've not written up the test plan for the new behavior yet. Feel free to do that and I'll post to the wiki. We'll need it to confirm the new and old behavior are working as expected.
Oops - I obviously did not test the original behaviour after the last few changes. I apologize.
Thanks for the test case - very useful.
I now have the original behaviour working properly. One slight change:
When stopping looping with a range set this range does not disappear. The range can still be used for cut/copy/clear. I think this makes sense as looping is not the only use of a range.
Please test with the newest version in the merge request. I will try to provide a test case for the new behaviour.
That's too big of a change. Current users are used to seeing the loop range come and go when looping is turned on and off. Need the original behavior restored. I've added test results to the test plan labeled as "bug1605 branch, 12 Jan 2022".
https://www.rosegardenmusic.com/wiki/test:classic_looping
For what it's worth, I can see the surprised users problem but I also think it sounds useful to have the range stay (since why would stopping change what is looped? Thinking from a conceptual & new user angle). If it is decided to be useful, from there I can see three possible ways to go about it:
I personally would find all of these ways ok, but that's of course easy for me to say being involved in seeing all this discussed and wanting many of these changes in the first place. I do personally hope however that no clear on stop can be worked in somehow at the end of the day, I think that sounds cool.
Made a first attempt at a test scenario for the new behaviour.
Please test.
I have restored the old behaviour (setting false).
I hope everyone will be happy now ;)
Please test again.
Classic Looping testing has passed. I'm going to have a look at the code changes next.
Looks good.
I've got some comments, but many are minor (standards) and I wanted to add some code comments as well. If you're out of there, I'll merge this and make the minor changes myself. Then I'll get started reviewing the new behavior and the test plan. Let me know if it's ok for me to take ownership.
That sounds fine to me. If you have some ideas what to do that is the best thing.
Please take over this bug.
Thanks
Merged as [1bb615]. Please test latest git.
Also, please review my cleanup commit: [177bbc].
Next for me is to review the new behavior before we announce this on the user list. I think I have some thoughts on improvements based on looking at Reaper to see where Ellie is coming from on this. Hopefully you will have time to implement them.
Related
Commit: [177bbc]
Commit: [1bb615]
I'm very late to this party, but now learning about the "Advanced Looping (experimental)" preference ("Nobody reads the release notes" -- thanks4opensrc, 2022) it's a huge improvement. I've never been able to imagine anyone having use for Rosegarden's continued playback to "infinity and beyond" after the last segment's end, and it always drove me crazy.
One more thing that I've been thinking about Feature Requesting but will piggyback here instead: Why does playback, when a loop range is set, jump to the beginning of that range? With no loop set playback doesn't jump back to the beginning of the composition. I expect it to always start at the current playback pointer -- unless that's outside the loop range, in which case it should jump to the beginning. Likewise, "Rewind to Beginning (Home)" and "Fast Forward to End (End)" should jump to the beginning/end of the loop range if one is set (which would take care of those who want to start playback at loop begin). And also: "FF to End" is still broken, jumping to the maximum measure number regardless of "Advanced Looping", whether a loop range is set, where the last segment ends, or anything else. That's useless, and any user who claims otherwise needs to have their head examined.
And finally, one very small point: I'd like to see the right-click to define the range always start in the "interval" defined by the tickmarks in the playback pointer ruler (always quarter notes, but come to think about it should be the grid spacing in the Matrix Editor). Right now if you click anywhere past halfway between the ticks and move forward, the highlight doesn't start showing and the range doesn't start until you move to the right past the next tick. I conceptualize the range to be the interval between the ticks ("all notes within this time range") so it always throws me off when I click past 50%.
Not a huge deal, and I can also see the argument that the range goes from one tickmark (a infinitely narrow geometric "point" in time) to another. Ironically what I'm asking for is the opposite of the one behavioral change I made in my implementation of Feature Request #501 / Merge Request #54 -- there, when editing a percussion segment the click goes to the closest grid line, to the right or left, because I claim that drum hits are on the beat as opposed to starting on it and continuing for some duration, like pitched notes do. I contend that's obviously the right thing in that case, but like I said I can see the argument going either way regarding the loop endpoints.
Thanks for listening. All these good improvements to Rosegarden, one baby step at a time.
Thanks for your contribution Mark.
I had a look at your merge request 74 and I had some problems.
Tirst the test
test_notationview_selection
now fails - I believe by accessing theSequenceManager
int theNotationView
constructor. Trymake test
in the build directory.Also I tried running the test case: https://www.rosegardenmusic.com/wiki/test:classic_looping and ran into problems. I got into a state where the looping button was pressed but playback was not looping. Perhaps you can try the test case.
Thanks for finding these problems, Philip.
Please accept my apologies for breaking
test_notationview_selection
. I hadn't looked into the unittest code and had gotten out of the habit of building and running it because I didn't think it tested GUI interactions like the changes in this merge request. Totally my mistake. The tests are great and many more of them are needed, although I still don't see how non-button actions, like rectangle-select in the editors, could easily be simulated.Of course the access of the null
SequenceManager
doesn't occur in normal RG execution, and fixing it for the unittest was simple. Again, thanks for pointing it out. Thereference_segment
test still doesn't compile, and since I didn't think that had anything to do with my code I tried it in master [f2516f], and it has the same failure there. So at least for now I'm putting it in a Hitchhiker's Guide to the Galaxy "somebody else's problem field" and leaving it alone. ;)Regarding the "classic looping" test wiki, I probably wasn't clear enough in my commit message documentation, but the code intentionally doesn't support the "classic" UI, nor even 100% match the existing "advanced looping". This touches on the much larger issue of changes to Rosegarden, which is a discussion I'd like to have on the
-devel
list. Briefly, it's my belief that changes are good if they're clear improvements and support all current useful (stress "useful") capabilities, even if with slightly different workflows or graphical displays. Of course I claim my merge code satisfies all of that, and although I've been trying to keep my changes to a minimum and support the old UIs (and keep them as the default; see things like the new track editor segment display in Merge#65), but that was impractical to do given the extent of the code changes here. Also, I consider what was removed to be "not useful", the primary example being the "classic" playback's continuing past the last segment's end time until the end of the composition. I always considered that to be a pure bug, but if anyone can provide a use-case argument for it (not merely "that's the way it's always been and users are used to it") I'd be interested in hearing it.If you have steps to reproduce this -- within the design and workflow of this new "advanced-advanced looping" merge -- please post them. Again as per the commit message documentation, the looping buttons (both the existing one in the transport dialog and the new ones in main, notation, and matrix) do not in themselves turn on looping over a specified time range. (They loop through a range only if one is defined and is active/displayed, though also through the entire range of segments if not.)
Regardless all the above, fixing the unittest breakage has given me the chance to go back and improve other things in the code, both internal and user-visible. For the latter I'm trying to pop up a warning dialog whenever the code is refusing to do something, such as if the user tries to define and/or activate a loop range when there are no segments in the composition. (That's an unlikely edge case, maybe when testing for the first time and clicking the transport dialog loop-range-setting buttons in a blank composition, but it's part of how the new design always clips loop ranges to the union of all segment's begin/ends.)
Regarding the code, it's doing the loop range clipping in one place in RosegardenDocument instead of repeatedly in LoopRuler (if multiple windows/LoopRulers exist). Also hopefully implementing the change to maintain some new RosegardenDocument private members for the begin/end segment's ranges as segments are added/deleted/changed instead of recalculating those values each time they're needed.
All of this has been extensive enough that I'd like to spend a day or so testing, but will push a new commit to my repo and update the merge request when I do. Thanks again for trying the code and finding bugs in my pre-merge commit.
Related
Commit: [f2516f]
No need to apologise for breaking the test - these things happen. Indeed the tests go further than classic unit tests and we should probably have more of them but as you point out some things are very difficult to test.
I am a little concerned about your problem with the reference_segment test. I can compile and run it OK. Can you specify the problem ?
There was a long discussion here with ell1e and Ted Feilx about the new functionality and maintaining old functionality. That is why there are two new preferences for "Stop at end of last segment" and "advanced Looping". We should respect these flags.
The "Classic Looping" test case is very valuable as it tests the old style classic looping and the new advance looping! Of course this test case is not engraved in stone but any changes do require a discussion. If you have suggestions for changing the test case please post them here.
To find problems just run the "New Looping behaviour" section of the test case - you will soon run into difficulties!
Giving a warning if a loop is outside the song range sounds like a good idea.
An initial response. More to come when I commit the latest fixes and improvements to my branch.
Regarding the test issue:
Yes. The latest test/reference_segment.cpp fails compilation on my Qt 5.9.7 system:
You'll probably immediately know what the problem is, but in case not, it's from the new test iin [7d6d7c3740] because 5.9.7 doesn't seem to have the required
QCOMPARE
overload:Regarding my changes:
I'd already fixed the bug where the transport dialog's "Start/End loop or range here" buttons hadn't been hooked up to the new looping code. When I finish the lastest revision I'll go back and run the the test steps to find if there are any remaining bugs (above and beyond cases where the new proposed design intentionally differs from the current "advanced looping").
Again, more to follow (for whatever it may or may not be worth), and I'll continue to look into the issue, but I'm fairly sure that it will be impossible to simultaneously support either "classic" or "advanced" looping in the same executable binary as my design. Likely the most workable path is something similar to what Ted suggests, below:
This probably also applies to my other extensive/invasive merge requests / branches, and further ones that I have planned for the near future. And as always, another acceptable alternative is that they merely stay forever in my own repo, regardless my opinions about their value and potential benefit to other RG users.
Thanks for the info.
Since we support Qt versions from qt-5.1.0 (do we really?) this is a legitimate bug. I have made a merge request to fix this ( https://sourceforge.net/p/rosegarden/git/merge-requests/75/ ).
The main branch supports advanced and old-style looping switched with a flag. Also the end playback at end of last segment switched with the preferences flag.
Maybe you can update your changes to use the flags as in the main branch ? It should be possible to support old and new in the same executable.
Plan for looping is as follows:
Seems like a good plan to me !!
The following is a response to a post on rosegarden-user, here instead of on the mailing list as was requested there. As always, these are my opinions and contributions and may be disputed, rejected, and/or ignored by any and all as they so choose.
On 6/20/22 5:44 AM, Ted Felix wrote:
Just to clarify, at least in the version I've submitted, it's "loop from the beginning of the earliest segment to the end of the last/latest one."
I agree that looping the whole composition is an edge case, but it's included to make the interface consistent (looping applies in the same way, either to the loop range if one is defined and is active, or else to the whole composition).
Yes, I think that's the main intent. In my version you can also play the loop range once each time the play button is pressed (or a keyboard shortcut, or double-click in the loop ruler) : "Let me hear that phrase ... hmm, good, but maybe if I changed ... let's hear it another time ..." as well as continuously.
I'm unclear here. My understanding is that looping is an editing/playback feature and is different than repeat segments in the composition.
My "it's useless" arguments include:
But, sigh, if this is the no-compromise sticking point, I'll try to incorporate it into my code. What I can't do is a runtime switch back to full "classic" looping behavior.
My version adds "loop" buttons to all windows (main, notation and matrix editors, transport, dialogs, etc.) all synced together for consistent visual feedback. This was motivated by the fact that I never use the transport dialog and it was a pain to have to bring it up to control looping. Different users, different workflows.
Since I've already done the massive code cleanup and simplification, and implemented almost all of the suggestions, I'm still hoping my changes will be considered and either merged, cherry-picked, or made available as a branch for testing.
The latest version is definitely looking better.
Some comments:
The stop at end of last segment flag is now used (good). However this also causes the playback to jump to the start of the first segment. I think this is wrong. For live performance it may be very useful to have some silence at the start of the composition. Also it is fairly simple (ctrl+a and move tool) to remove unwanted space at the start of the composition. I think the jump to the start of the first segment is not such a good idea.
The behaviour with the advanced looping flag off does not correspond to the test case. How bad is this ? I am not sure. The idea was to maintain the old behaviour if the flag was false. What do others think about this ?
The "Invalid loop range" warning has a "Don't show this warning" CheckBox (Maybe should be "Don't show this warning again"). If I click this the warning still shows again after restarting rosegarden. The AudioFileLocationDialog also has such an option - but it is remembered in the preferences.
With advanced looping on and the whole song looping - if I create a range with right click and drag I get some strange behaviour: I cannot seem to reproduce this. I have seen the following:
1. Playback just stops
2. The loop is ignored
3. Playback jumps to the start of the loop.
The agreed functionality (see discussion with ell1e) was that the song will continue playing until it enters the range and then loop in the range.
Thanks for the quick feedback, Philip. I was going to post something about my latest commit but you beat me to it. Responses to your comments below, but first a recap on what I've submitted:
This latest commit adds:
Recording, both normal and punch-in, should be unchanged. The slight modification to the above playback description is if "Stop playback at the end of last segment" preference is unchecked, playback with no loop range continues past the last segment to the composition's last measure.
There are some slight inconsistencies with what happens in extreme edge-case scenarios that haven't been discussed here, things like starting playback with the loop range inactive and then activating it (with the cursor outside that range), or "jumping" the cursor in and out of the range while playback is in progress. Nothing "bad" should happen, but there is different behavior depending on whether the cursor is before vs after the range.
I'm not able to perfectly fix this within the current code architecture, where playback and looping control is scattered at random across methods in the
RosegardenMainWindow
,SequenceManager
,RosegardenSequencer
, and even the driver classes. I'm tempted to rip this all out and have it solely inRosegardenMainWindow::slotSetPointerPosition()
-- or preferably in a new class/.cpp/.h file -- but am leaving it as-is for now. Any thoughts on why it couldn't be done are welcome.Philip Leishman wrote:
I don't understand your point. Are you saying looping should not loop the whole composition (the segments)? That clicking play at when at the end shouldn't jump to the beginning? Both are intentional, matching the "range and segments behave the same" and "repeat or play once" design. Like the "continue past last segment" preference I see no use for playback doing nothing if clicked a the end (and if that also applied in the loop range case it would break the IMO very useful "play the loop once, then click to play it again" functionality).
Again, my position is that if the performance is supposed to have four measures of silence at the start they should be in the segments/notation. It's easy enough to use the "Resize" tool to extend one of them backwards.
As I said before, I've made no attempt to support "classic" looping. Time, and software, moves on. The old looping interface was barely usable, and this new code (mine or otherwise) is a huge improvement. If anyone presents a plausible use-case scenario that it doesn't support I'll be happy to consider adding it, but if it's merely the fear that some hypothetical user will go "I loved the old way and can't use this new stuff" I'll leave it to another contributor to hack the implementation (mine or otherwise) with the required huge amount of bug-inducing alternate code paths.
Funny you should say that. It was "Don't show this warning again" right up until I did the commit and reconsidered, precisely because it was confusing ("Don't show again ever" vs "Don't show again for the remainder of this session").
I intentionally chose the latter for a number of reasons. The warnings only pop up when the user makes a mistake (creating a range outside the segments) or in rare situations (moving segments away from the loop range). They're only there because software should never do something contrary to what the user requested without notifying them why. My expectation is that after seeing the warning once a user will rarely do something to trigger it again, and the "don't show again" button is there just so the warnings aren't annoying if a second mistake is made. They're not saved across sessions because I haven't liked it when software does that and I've thought, "Wait a second, I used to see some kind of message about this but now I don't and I can't quite remember what it was." I suppose there could also be a "Re-enable all warnings" button in preferences, but at least for me I never found anything in preferences without first reading documentation about it somewhere, so I think as a user I'd still be puzzled. But once again, for now I'll leave the decision, and implementation, to someone else.
Did the description of the playback problems above explain any of this? I haven't seen anything non-deterministic in my testing but I'll keep an eye out for it. Also, I don't understand -- do you mean you see this immediately upon right-click-drag creating a range, or only if you create the range that way (as compared to the other, less common UI methods) and then do playback?
I understand, but I think having a consistent and useful interface is more important than supporting idiosyncratic use-cases ("I want a lead-in"). OK, but I want to hear the loop without one. BTW, it turns out that lead-in is somewhat supported in my version: Define the range including the lead-in, start playback, and when the cursor moves past the desired loop start point, right-click-drag the start to there. The last comment from ell1e was 5 months ago -- perhaps they could try my or some other more recent commit and provide updated feedback.
A true bug should just be fixed.
This is really more of a feature request (although filed as a bug).
If new features are implemented they should normally be configurable. Rosegarden has a lot of users and they should not be surprised by functionality just changing. I believe this is the philosophy behind using the preferences to maintain old functionality and having a flag to activate the new behaviour
The main philosophy behind the advanced looping in the main branch is that the cursor should not jump into a loop if it would get there by waiting. I think your version of advanced looping is something completely different. Maybe it can be implemented by replacing the "Advanced looping" checkbox with a combobox with three options. But again - it should be configurable.
Maybe we are just going to have to agree to disagree !!
Sorry I didn't have time to test it yet, got dragged into some things.
However, I agree that at least when I proposed it I also thought there should be no unnecessary jumping, because it allows doing a sort of uninterrupted going-with-the-flow live'ish vibe&compose style where you want the beat to not do weird skips. And if you wanted to fast forward, you could still click to jump after all.
Whether this is how the majority would find it useful, I don't know. But I'd rather not have the program do too much for me that I may not actually want, when there is such an
easy way to do it manually if I wanted to (one click). Just seems more flexible to different workflows to me.
Philip Leishman wrote:
Yes, that's probably the bottom line. The following are merely details ...
ell1e wrote:
In principle I agree, the difference here being the "unnecessary" part.
Beating the dead horse of what I've already said, if I've defined and activated a loop, I want to hear that loop. And in my version it's very obvious when a loop is defined/active, even if that loop's time range is scrolled offscreen, so there are no "surprise" jumps.
I'm not sure if you're talking about generic jumping to a time, or specifically to the beginning of the loop, i.e. moving the cursor by hand if not using my version's automatic jump.
If the latter, that's the reason for the jump: Otherwise it's extra effort to get the cursor precisely on the loop start, especially if the start isn't exactly on a beat (yes, I know, unlikely, but still).
The counterargument might be a desire to do what I'll call "Q"-looping (a loop with a "pre-tail") vs plain "O"-looping. I.e. a loop with a lead-in from the current cursor time. This is what I think is being said about playback until the loop is reached, and then looping within the range.
To my mind this is a much more idiosyncratic workflow than plain "O"-looping. What about if the cursor is past the loop? Should playback go to the end (the end of the last segment, not the crazy "to the maximum measure number" -- at least we agree about that), loop back to first measure, play until the loop is reached, and then loop? As I've said, the basic, "start before the loop" "Q"-loop can be done in my version by starting playback, waiting until the loop is entered, and then activating it.
The most important point is that the jumping allows single playback of the loop range, once each time on command, which I find very useful for reviewing a phrase or a passage. And it's ironic that I undertook the changes (which were much more difficult than I'd anticipated) exactly because the current looping jumps to the start of the loop if playback is started in the middle of it. I do want playback exactly where I've set it ... including when that "setting" is to a loop and I'm not already there.
Finally, from a code and versioning standpoint, doing the "Q"-loop, at least in its basic form, would be a one line change in the version I've presented. Plus if using my next version, Feature Request #508, there's even a new Preferences implementation that makes adding efficient/cached preference access trivial. Note however that doing any "post-Q-looping", or particularly supporting the original "non-advanced" looping would probably be much more difficult. I'm not really sure as I don't have the time or interest in historic implementations that in my opinion should be forgotten/abandoned, so I haven't looked into it.
All I can do is offer up the work I've done and hope that people will try it and evaluate it based on its merits. I've faced this myself with software I use: When zsh added the extra "enter" required when doing a cut-and-paste into the shell I first objected, but then after using it for a very brief time realized what a good idea it was. In any case, going back to Philip's comment quoted above, maybe a code divergence is unavoidable. But that's the subject of another discussion somewhere else.