Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use mean based logic for controlling loop points. #4034

Merged
merged 6 commits into from
Jul 29, 2019
Merged

Use mean based logic for controlling loop points. #4034

merged 6 commits into from
Jul 29, 2019

Conversation

husamalhomsi
Copy link
Member

@husamalhomsi husamalhomsi commented Dec 5, 2017 •
edited
Loading

mean_loops
With this, right clicking the time line widget will set the nearest loop point.
Additionally, it only allows setting loop points with right clicking.

@Spekular
Copy link
Member

Spekular commented Dec 5, 2017 via email

@qnebra
Copy link

qnebra commented Dec 5, 2017

Looks way more intuitive than current loop points logic.

@vlad0337187
Copy link

As I understood, you divided distance between loop points on 2 parts.
If somebody click at the left one - than will be moved left loop point, if on right one - right.

If some - than I agree, it's a better behaviour. +1
Because I always confuse on which button to press to move appropriate loop point =)

Limit controlling loop points to right mouse button.
e506b90 
To avoid confusion.
The middle mouse button could control other elements of the time line.
@husamalhomsi
Copy link
Member Author

I've also suggested Shift+L/R to directly set the start/endpoint in a manner similar to the current behavior. That way those who don't like the new behavior can use something similar to the current/old way.

@Spekular The new behavior is much better than the old behavior, so I think all users will like it.

@Wallacoloo
Copy link
Member

Just gave this a try, and I like it. @Spekular, did you have some hang up with this change, or can we merge this?

@Spekular
Copy link
Member

Spekular commented May 9, 2018 via email

@Wallacoloo
Copy link
Member

@Spekular Okay, if there's a clean way to do that, I won't argue.

I'm pretty sure the existing behavior is normal right-click moves the loop endpoint, and shift+right-click moves the loop start point. Ctrl disables the magnetic behavior. So both the shift and ctrl modifiers are in use. I guess we could somehow throw Alt into the mix, but that many modifiers for something seems excessive. We could tuck away an "Enable advanced keybindings" options in the settings somewhere to control which set of keybindings are enabled.

I'm not particularly fond of adding another setting like this, but also I really like the new behavior and would love if there's a way to get it in that keeps older users happy.

Can you think of a good way that the old behavior could still exist alongside this one?

@Spekular
Copy link
Member

Spekular commented May 9, 2018 via email

@vlad0337187
Copy link

vlad0337187 commented May 9, 2018 •
edited
Loading

I use left-handed mouse configuration, so pressing LMB / CMB always confused me...
Using one mouse button for all would be great, as I think.

@Wallacoloo
Copy link
Member

@Spekular Ah! Sorry, I misunderstood your original post - thanks for having the patience to reexplain it.

@Sawuare I know it's been ages since you opened this PR, but if you have an opportunity sometime to adjust the behavior to match Spekular's last comment, then I think we could get this merged.

@husamalhomsi
Copy link
Member Author

Shift+LMB is used for SelectSongTCO. I think it should be changed to Ctrl+LMB, to leave Shift for the old loop behavior. Thoughts?

@Wallacoloo
Copy link
Member

Wallacoloo commented May 10, 2018 via email

@Spekular
Copy link
Member

Spekular commented May 10, 2018 via email

@Spekular
Copy link
Member

Spekular commented May 10, 2018 via email

@husamalhomsi
Copy link
Member Author

@Spekular Okay, if there's a clean way to do that, I won't argue.

@Wallacoloo I tried and it does not even work, let alone being a clean way.
No users mentioned disliking the new behavior, so I think the old one should be removed.

@husamalhomsi husamalhomsi changed the title Use median based logic for controlling loop points. Use mean based logic for controlling loop points. May 12, 2018
@zonkmachine
Copy link
Member

@Sawuare The new behavior is way better... until you zoom in on a part and want to set new loop points. Can the selection be relative to the looped area visible when not both loop points are visible?

@husamalhomsi
Copy link
Member Author

@zonkmachine I think it would be confusing, plus it's just an extra click to move the loop point near the desired position before zooming in and making fine adjustments.

@qnebra
Copy link

qnebra commented May 6, 2021

I had even better idea. Revert every UX change from last 15 years in lmms.

  • right mouse button - agree, it can be problematic in device without dedicated RM button, like Macbook or those with touch screen.
  • mean logic - if user is unable to perform basic estimation then something is really wrong with him.

@Veratil
Copy link
Contributor

Veratil commented May 6, 2021

Some like this change and some hate it, so I will revert it for now and maybe offer it as an alternative in the future.

I think we should get a poll on this before we revert it. How many like the new way? How many like the old? Is this or the previous way more intuitive? Is there a way to make this way better?

I had even better idea. Revert every UX change from last 15 years in lmms.

This kind of comment is not needed.

@rdrpenguin04
Copy link
Contributor

I honestly prefer the old way because, for longer loops, I now have to zoom out or scroll a long ways to move one end of the loop. This is primarily an issue when I'm trying to loop a small section to work on balance or tune a synth.

OTOH, it was inconvenient to remember which button combination moved which side of the loop.

@tresf
Copy link
Member

tresf commented May 6, 2021

I agree, with all comments, especially @Veratil's proposal to make the existing behavior palatable. Interestingly enough, this regression was predicted by @Spekular and a few others, quoting:

@Spekular wrote:

@Umcaruje clicking in a certain region only selects that handle, after that it can be dragged. I've also suggested Shift+L/R to directly set the start/endpoint in a manner similar to the current behavior. That way those who don't like the new behavior can use something similar to the current/old way.
...
I think it would be best to allow the old method using a modifier. While the new method is great, the old method can be faster (any time you wish to set a loop point within the selection range of another), and in certain situations even more convenient.

@Wallacoloo wrote:

I'm pretty sure the existing behavior is normal right-click moves the loop endpoint, and shift+right-click moves the loop start point. Ctrl disables the magnetic behavior. So both the shift and ctrl modifiers are in use. I guess we could somehow throw Alt into the mix, but that many modifiers for something seems excessive. We could tuck away an "Enable advanced keybindings" options in the settings somewhere to control which set of keybindings are enabled.

I'm not particularly fond of adding another setting like this, but also I really like the new behavior and would love if there's a way to get it in that keeps older users happy.

Can you think of a good way that the old behavior could still exist alongside this one?

... and then the conversation circles around which keyboards shortcuts are even possible.

@douglasdgi writes:

Tested it out. I like the new behavior. I did run into the same problem @zonkmachine ran into with not being able to easily set the loop points where I wanted them while zoomed in, but I can't think of a better behavior that would work without confusing some people.

My vote is the following:

@lookitsnicholas, open a new bug report with a screencast of the exact problem. Tag those people here that seem vested so they're subscribed.

We approach it two ways, these are NOT mutually exclusive:

  • Add back some bindings to keep the old behavior

    -- AND --
  • We look to see if something can be done to fix the zoomed-in behavior

@qnebra
Copy link

qnebra commented May 6, 2021

I had even better idea. Revert every UX change from last 15 years in lmms.

This kind of comment is not needed.

Because it is pointless comment. Always someone complains when some changes to UX are made, so best way to avoid that will be simply to not do any changes.

I have no idea if it is ever possible to do, to set loop points based on currently visible section of timeline in Song Editor. I guess it would be really complex in code.

@tresf
Copy link
Member

tresf commented May 6, 2021

Because it is pointless comment. Always someone complains when some changes to UX are made, so best way to avoid that will be simply to not do any changes.

The person posting -- also a code contributor -- is making a usability argument for the old behavior. I too use a Macbook for production. Let's be careful how dismissive we are to our users, especially those willing to submit patches. 🍻

@RoxasKH
Copy link
Contributor

RoxasKH commented May 6, 2021 •
edited
Loading

The principal fact for why this change can be not very well accepted by people is actually because it's a change.
I mean, i struggled with it, too, but only because as an old user i got used to the old logic.

Let's be honest, tho, if we're talking about being intuitive, the new behaviour wins. Yes it can be a bit uncomfortable in doing things, but let's think about new users: moving the left point is way more accessible now.
I don't really see this as a regression, sometimes it needs some more tweaking, but it's still very functional.

The main point while this kinda alienate me is because i'm used to the old one to the point is become something i do without even thinking.
It doesn't mean i can't for example get used to the new one, which can also be more comfortable in other cases.

Even if key shortcut are essential in music productioni don't know if the shift-click was better or not.

@qnebra
Copy link

qnebra commented May 6, 2021 •
edited
Loading

And I agree with that point about usabiity in some cases. Unlike in moment of writing first comment in this PR I currently had laptop without dedicated RMB, and luckily for me I had connected mouse to it. Without it using this scheme of control will be more difficult. Also, what if in one point in future timeline got functions which requiered access to pop-up menu? Like markers for example.

But revert entire PR because someone is used to old way and want to preserve old behaviour forever?

@tresf
Copy link
Member

tresf commented May 6, 2021

But revert entire PR because someone is used to old way and want to preserve old behaviour forever?

I think we can all agree, reverting the feature in its entirety is a bad idea.

@RoxasKH
Copy link
Contributor

RoxasKH commented May 6, 2021

Probably we can keep implemented the shift shortcut for moving only the left loop point, can we?

It wouldn't be a complete revert, but it's still something

I dunno how much consistent it'd be tho

@akimaze
Copy link
Contributor

akimaze commented May 7, 2021

Why not add an option in the settings? If each of the approaches has its advantages and disadvantages and it is difficult to decide, it may be worth putting the decision in the hands of the user.

By default, this may be a new behavior because it is easier to discover.

In my opinion, the old solution was much more effective. Now I often have to make a lot more moves to make sure the correct point gets changed. Especially when I don't see the starting and end point. It happens (quite often) that the point that I do not want changes and then I have to set two points again. This is quite annoying.

New solution is better when you see start and end point. When loop is bigger old solution wins.

@akimaze
Copy link
Contributor

akimaze commented May 7, 2021 •
edited
Loading

Another solution could be a graphical representation of which point will change when we move the cursor over the loop bar. Then I would know if I need to move the cursor somewhere else to achieve my goal. Something like that:

image

@RoxasKH
Copy link
Contributor

RoxasKH commented May 7, 2021

Why not add an option in the settings? If each of the approaches has its advantages and disadvantages and it is difficult to decide, it may be worth putting the decision in the hands of the user.

By default, this may be a new behavior because it is easier to discover.

In my opinion, the old solution was much more effective. Now I often have to make a lot more moves to make sure the correct point gets changed. Especially when I don't see the starting and end point. It happens (quite often) that the point that I do not want changes and then I have to set two points again. This is quite annoying.

New solution is better when you see start and end point. When loop is bigger old solution wins.

The new one requires zooming out, is true, but it's just another passage to do.

If dev were to add an option in the settings for ever change they made the settings would be an infinite list of things, so i don't know if it's the right way to go.

@akimaze
Copy link
Contributor

akimaze commented May 7, 2021 •
edited
Loading

The new one requires zooming out, is true, but it's just another passage to do.
If dev were to add an option in the settings for ever change they made the settings would be an infinite list of things, so i don't know if it's the right way to go.

In my opinion, developer should make this decision (should there be an option?) every time it is necessary. Each case/functionality is different, so there is no precedent law (if we add option once we must add option for every subsequent change). Also, there is no obligation that each task can only be done in one way if there are arguments/other scenarios for it.

Option can be removed when new new solution will work fine in any scenario.

@lookitsnicholas
Copy link

lookitsnicholas commented May 7, 2021 •
edited
Loading

Another solution could be a graphical representation of which point will change when we move the cursor over the loop bar. Then I would know if I need to move the cursor somewhere else to achieve my goal. Something like that:

image

This seems like a Band-Aid (if anything else) because it still requires that the user has to perform additional movements based on the dynamic UI.

My overarching issue with this PR is not necessarily the fact that the previous behavior is changing, it is that it is breaking the objectively more efficient route with seemingly no initial reason for it to exist in the first place (which the counterarguments do not seem to directly address).

If discoverability was indeed somehow the main issue, then why not add a tooltip specifying the old control scheme upon hover?

Again - if there is a lot of support for this behavior existing, then I would present it as an option instead of destructively modifying the existing behavior without significant feedback.

I will get to creating the new issue in a bit, I just have other things to do in the interim.

@Spekular
Copy link
Member

Spekular commented May 7, 2021

I will get to creating the new issue in a bit, I just have other things to do in the interim.

There's no need, there's already an open issue for it: #5505

@lookitsnicholas
Copy link

lookitsnicholas commented May 7, 2021 •
edited
Loading

I will get to creating the new issue in a bit, I just have other things to do in the interim.

There's no need, there's already an open issue for it: #5505

That was per suggestion from a previous reply, not sure if any traction was garnered in relation to that particular report.

@tresf
Copy link
Member

tresf commented May 7, 2021 •
edited
Loading

There's no need, there's already an open issue for it: #5505

That was per suggestion from a previous reply, not sure if any traction was garnered in relation to that particular report.

I didn't realized #5505 existed. @JohannesLorenz actually drew the issue pretty clearly using some ascii characters , so probably no need for a screencast (or we can edit his to include yours).

What if the mean-based logic was changed to be based on the viewable region, not the entire region?

@lookitsnicholas
Copy link

What if the mean-based logic was changed to be based on the viewable region, not the entire region?

Maybe I'm missing something, but how would that even work if the loop is located near the extreme of one side of the screen?

@tresf
Copy link
Member

tresf commented May 10, 2021

Maybe I'm missing something, but how would that even work if the loop is located near the extreme of one side of the screen?

Actual:

<----------------------------------->
                    ^------- mean

Viewable:

<----------------->
          ^------ mean

Un-viewable:

                          <-------------------->
   ^------- (no mean, like clicking new)

@lookitsnicholas
Copy link

lookitsnicholas commented May 10, 2021 •
edited
Loading

So, zoom actions would be required from the user in certain cases (see: widescreen monitor). Additionally, that does not seem to address the issue that would arise from odd-numbered measure lengths - unless the user zoomed in such that the loop filled the entirety of the screen, multiple inputs would still be necessary in order to move the right marker to the left-hand side without issues (depicted below).

1      2      3      4
<-------------------->
       ^------ desired loop end point

What I had presumed was being suggested earlier was somewhat the opposite train of thought, where the mean-based logic was only ever active when the viewable region was less than the entirety of the track (which would be significantly less than ideal, as well).

@qnebra
Copy link

qnebra commented May 10, 2021

I feel like best in terms of intuitive use will be "vieweable mean". It requiered only to scroll to desired part in any zoom level, and loop points can be set without further scrolling. I think in my opinion lmms UX should be easy to discover and fun to use.

@tresf
Copy link
Member

tresf commented May 10, 2021

Instead of arguing what will work, I'd recommend opening a new bug report and just describing what you want. A picture is worth 1,000 words. Describe the current behavior, the desired behavior and how you'll fix it.

For example, GarageBand just allows you to click and drag your region, then use shift to place left and right at the cursor. It's intuitive, works for millions of users, doesn't require any complicated memorization of shortcuts or oddball mouse buttons that don't exist on laptops. GarageBand also allows loop toggling based on click on/off.

I'm not stating Garageband is the holy grail of usabilby, but at some point we should just accept human interface decisions made by others that are good enough for the masses.

@tresf
Copy link
Member

tresf commented May 10, 2021

ezgif-6-3f1851913d0f

@tresf
Copy link
Member

tresf commented May 10, 2021

ezgif-6-efe1eb783195

@qnebra
Copy link

qnebra commented May 10, 2021

But, what can be guessed from my first comments under this PR, I really liked "mean based logic" introduced in this PR and found it really easy to use. This behaviour in Garageband looks quite intuitive.

I think main arguments here are:

  • it is new behaviour, I didn't like it, give me back old behaviour
  • it is wacky in some cases

How Garageband behave when for example loop 'end point' is far away in timeline? Like in measure 500 when visible are those from 1 to 50.

@tresf
Copy link
Member

tresf commented May 10, 2021

How Garageband behave when for example loop 'end point' is far away in timeline?

Alt modifier combined with Shift will override mean guessing.

ezgif-6-801bab4ed171

@lookitsnicholas lookitsnicholas mentioned this pull request May 11, 2021
Closed
@Xeno-Idaltu

This comment was marked as abuse.

Sign in to view
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
@husamalhomsi
Use mean based logic for controlling loop points. (LMMS#4034)
83942e4 
* Use median based logic for controlling loop points.

* Limit controlling loop points to right mouse button.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.