Improving movement inside workspace #7595

szeli1 · 2024-11-23T11:48:18Z

This Pull Request adds the ability for users to move inside the workspace without using the scroll bars.

This change allows the user to click on the workspace (where the editors are located) without clicking on a SubWindow, and move by mouse drag. I believe this improves the workflow significantly, because instead of needing to move the mouse over to the scroll bars, the user can simply click on the workspace background and move both vertically and horizontally.

As far as I know there is no Issue open referencing this feature.

sakertooth

Tested it and it works fine, just some code suggestions.

include/MainWindow.h

src/gui/MainWindow.cpp

include/MainWindow.h

sakertooth · 2024-11-23T17:40:52Z

Hm, the build is failing (I'm guessing Carla isn't linked to lmms). Try moving the implementation of MainWindow::workspace back into the header file.

include/MainWindow.h

DomClark · 2024-11-23T22:02:22Z

Should there be a cursor change associated with panning the workspace, perhaps?

szeli1 · 2024-11-23T22:17:50Z

Should there be a cursor change associated with panning the workspace, perhaps?

Sure! What type of cursor should be displayed? Maybe Qt::ClosedHandCursor?

sakertooth · 2024-11-23T23:09:47Z

Probably Qt::OpenHandCursor and Qt::ClosedHandCursor yeah.

regulus79 · 2024-11-24T03:00:15Z

I like it. Trying it out, I feel like if the scroll limits were removed/expanded, it would feel more natural to be able to drag the view "out of bounds", instead of being capped to the position of the min/max windows. You don't have to implement that though, I'm just thinking out loud.

szeli1 · 2024-11-24T11:45:40Z

I feel like if the scroll limits were removed/expanded

I don't know how to do this unfortunately, it seems like SubWindows resize the scroll bars when they are moved. An other method of moving exists for QMdiArea: scrollContentsBy(), but this doesn't resize the scrollarea as far as I know, and I couldn't get it to work with the scrollbars.

szeli1 · 2024-11-24T11:47:45Z

Probably Qt::OpenHandCursor and Qt::ClosedHandCursor yeah.

I implemented Qt::ClosedHandCursor while dragging.

sakertooth · 2024-11-25T14:32:55Z

Curious why Qt::OpenHandCursor is not being used.

sakertooth · 2024-11-25T14:48:41Z

I'm also thinking this feature should be opt-in.

szeli1 · 2024-11-25T15:54:19Z

Curious why Qt::OpenHandCursor is not being used.

I felt that it is unnecessary and could be annoying because the workspace covers 80% of the screen and the default cursor looks better in that case, but I could implement it if needed.

I'm also thinking this feature should be opt-in.

I think users could opt out by not using this feature, I believe there is no point to make this optional if it is an improvement for all platforms and I believe it is.

sakertooth · 2024-11-25T16:03:11Z

I felt that it is unnecessary and could be annoying because the workspace covers 80% of the screen and the default cursor looks better in that case, but I could implement it if needed.

Okay. In my opinion, not having Qt::OpenHandCursor makes it hard for people to discover the feature because its not intuitive. Users may end up still using the scrollbar because they had no idea you could simply drag the workspace. The Qt::OpenHandCursor gives the user that indication so they will know that you can do such an action.

I think users could opt out by not using this feature, I believe there is no point to make this optional if it is an improvement for all platforms and I believe it is.

Okay. However, you may believe it is an improvement, but certain users may feel differently. It's a possibility. I also think its an improvement, but I do want to avoid any complaints being sent in about it being a forced feature upon users. That probably wont happen, but there's always a chance.

Edit: Actually, I thought about it a bit more. I do agree that the feature can be opt out as well.

szeli1 · 2024-11-25T18:07:42Z

Edit: Actually, I thought about it a bit more. I do agree that the feature can be opt out as well.

~~So you agree with yourself?~~ Edit: I will add an opt out setting.

Okay. In my opinion, not having Qt::OpenHandCursor makes it hard for people to discover the feature because its not intuitive. Users may end up still using the scrollbar because they had no idea you could simply drag the workspace. The Qt::OpenHandCursor gives the user that indication so they will know that you can do such an action.

I will implement this then.

sakertooth · 2024-11-25T21:48:54Z

If you are not truly satisfied with the suggestion @szeli1, I think another option we can do is to keep the arrow cursor when the user is not pressing down on anything.

Then, you could use the open hand cursor when the user initially presses down on the mouse, and when dragging, use the closed hand cursor. When they stop dragging but are still holding the mouse, switch back to the open hand cursor, and when the mouse is released, move back to the arrow cursor.

You might not even need to add an opt-in/opt-out setting actually if you don't want to, as I think discovering the feature by pressing down on the workspace is enough if the cursor switches are implemented the way I described it above.

I think this is somewhat subjective ultimately, but let me know what you think of this idea.

szeli1 · 2024-11-26T21:33:23Z

When they stop dragging but are still holding the mouse

This would require a timer, because MouseMoveEvent is only called when the mouse moved. Unfortunately I don't see why your method is an improvement and how this would help discovering the feature. (Also the Mdi windows don't change the cursor when they are moved, so I don't think it is a big problem if the workspace doesn't indicate it, the communication around this would need to change in documentation and tutorials)

Edit: please let me know if you approve this or you would rather like your idea implemented.

You might not even need to add an opt-in/opt-out

I think it was a good suggestion to add an opt out option, since it will enable users who really don't like the feature to turn it off, it will leave the decision to the user.

sakertooth · 2025-02-21T23:13:49Z

Edit: please let me know if you approve this or you would rather like your idea implemented.

IIRC this PR should be ready for merge. There are small things with the icons I wanted to change but nothing major and potentially unwanted. We need a more recent test, but I won't be able to test this unfortunately so I would recommend finding someone new to take on the testing torch for this PR.

regulus79 · 2025-03-01T16:55:51Z

I tested this again after the last commit, and it's awesome. I love that the view is no longer bounded by the scroll bars. It feels so much more natural.

regulus79 · 2025-03-01T16:58:30Z

src/gui/MainWindow.cpp

+			minX = minX > curWindow->x() ? curWindow->x() : minX;
+			maxX = maxX < curWindow->x() + curWindow->width() ? curWindow->x() + curWindow->width() : maxX;
+			minY = minY > curWindow->y() ? curWindow->y() : minY;
+			maxY = maxY < curWindow->y() + curWindow->height() ? curWindow->y() + curWindow->height() : maxY;


Is it better to do this or use std::min/std::max?

I believe it is slower, since it is a function instead of this 1 line if statement (it is slower because the code after this kind of if statement is loaded in by the cpu, I believe the min max function would make the cpu jump to a different memory address, so it will load slower)

I believe it is slower, since it is a function instead of this 1 line if statement (it is slower because the code after this kind of if statement is loaded in by the cpu, I believe the min max function would make the cpu jump to a different memory address, so it will load slower)

Is the function call overhead really that slow to matter? I think the readability benefits from using std::min and std::max in this case are worth it, and this isn't performance critical code.

Wait, just checked the definition:

template<typename _Tp, typename _Compare> _GLIBCXX_NODISCARD _GLIBCXX14_CONSTEXPR inline const _Tp& min(const _Tp& __a, const _Tp& __b, _Compare __comp) { //return __comp(__b, __a) ? __b : __a; if (__comp(__b, __a)) return __b; return __a; }

There isn't even function call overhead because it's inlined (I would also expect most implementations of these functions in other compilers are also inlined). You also get the added benefit of being able to evaluate the calculation at compile time if the arguments passed in allow it.

I think the ternary operator will be faster than if, because as I've said there is not much branching. In assembly it will skip 1-2 line if false, this makes it so that the code is loaded in faster and maybe more predictable, so the cpu will optimize it. This is how I understand how the ternary operator works, I may be wrong tho

I will replace it with std min max

A ternary operator is a branch. if-else is a branch. They are both branches, but different syntactically is all.

If we didn't have to pay for branch mispredictions when using the ternary operator, this would be insanely overpowered.

If we didn't have to pay for branch mispredictions when using the ternary operator, this would be insanely overpowered.

I mean it is a branch, a branch is essentially a subtraction and a change to maybe the program counter that decides what is the next executed line (in arm assembly). I believe ternary operators are faster because only 1 branching instruction is called after compare, so there is only 1 potential jump between where the code gets executed, and the program counter will stay on the same branch.

Do you have any evidence that a ternary operator is faster than a regular if-else branch?

szeli1 · 2025-03-01T22:25:55Z

I replaced the ternary operator with std::min and std::max

sakertooth

Code looks fine, and its working as expected. However, do we want to remove the scrollbars? I'm not too sure if we should keep them or toss them personally.

szeli1 · 2025-03-02T12:29:18Z

Code looks fine, and its working as expected. However, do we want to remove the scrollbars? I'm not too sure if we should keep them or toss them personally.

Scroll bars don't work after this change.

sakertooth · 2025-03-02T12:32:59Z

Scroll bars don't work after this change.

I was looking for reasons why it was removed, but I'm guessing it was removed because they got in the way, which is a fair justification 👍

#7752) Fix bug introduced with #7595 where drag-moving the MDI canvas background would break the show/hide behavior of a fullscreen window. Closes #7751

LMMS#7752) Fix bug introduced with LMMS#7595 where drag-moving the MDI canvas background would break the show/hide behavior of a fullscreen window. Closes LMMS#7751

szeli1 added 3 commits November 23, 2024 12:33

MainWindow_implementing_custom_QMdiArea

f480848

MainWindow_style_changes

e857a2f

MainWindow_removing_comment

7f0a2fd

sakertooth reviewed Nov 23, 2024

View reviewed changes

include/MainWindow.h Outdated Show resolved Hide resolved

src/gui/MainWindow.cpp Outdated Show resolved Hide resolved

src/gui/MainWindow.cpp Show resolved Hide resolved

MainWindow_fixing_casting_and_style

89cc7a1

szeli1 requested a review from sakertooth November 23, 2024 16:19

szeli1 added 3 commits November 23, 2024 17:27

MainWindow_fix_casting

ef03777

MainWindow_moving_class_and_fixing_casting

477600c

MainWindow_renaming_CustomQMdiArea_to_MovableQMdiArea

8480d1c

sakertooth approved these changes Nov 23, 2024

View reviewed changes

include/MainWindow.h Show resolved Hide resolved

MainWindow_moving_function

72c3e45

sakertooth reviewed Nov 23, 2024

View reviewed changes

include/MainWindow.h Outdated Show resolved Hide resolved

MainWindow_changing_cursor

cef80a6

MainWindow_removing_code_from_constructor

2171488

MainWindow_reworking_feature_to_not_use_scroll_bars

151ca4b

regulus79 reviewed Mar 1, 2025

View reviewed changes

MainWindow_replacing_ternary_operators

351d439

sakertooth reviewed Mar 2, 2025

View reviewed changes

sakertooth merged commit 5fa01e7 into LMMS:master Mar 2, 2025
10 checks passed

bratpeki mentioned this pull request Mar 3, 2025

[Bug] Fullscreened subwindows not centered after #7595 #7751

Closed

tresf mentioned this pull request Mar 17, 2025

Off-screen windows should be more obvious when shown #7790

Open

szeli1 mentioned this pull request Apr 3, 2025

MainWindow revert to scrollbars #7828

Open

sakertooth mentioned this pull request May 31, 2025

Right clicking Equalizer permanently changes mouse icon to grab #7917

Open

1 task

messmerd mentioned this pull request Oct 16, 2025

[detach-window] Attach windows on close instead of show #8089

Merged

Uh oh!

Improving movement inside workspace #7595

Improving movement inside workspace #7595

Uh oh!

Conversation

szeli1 commented Nov 23, 2024

Uh oh!

sakertooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sakertooth commented Nov 23, 2024

Uh oh!

Uh oh!

DomClark commented Nov 23, 2024

Uh oh!

szeli1 commented Nov 23, 2024

Uh oh!

sakertooth commented Nov 23, 2024

Uh oh!

regulus79 commented Nov 24, 2024

Uh oh!

szeli1 commented Nov 24, 2024

Uh oh!

szeli1 commented Nov 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sakertooth commented Nov 25, 2024

Uh oh!

sakertooth commented Nov 25, 2024

Uh oh!

szeli1 commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sakertooth commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szeli1 commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sakertooth commented Nov 25, 2024

Uh oh!

szeli1 commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sakertooth commented Feb 21, 2025

Uh oh!

regulus79 commented Mar 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szeli1 Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szeli1 commented Mar 1, 2025

Uh oh!

sakertooth left a comment

Choose a reason for hiding this comment

Uh oh!

szeli1 commented Mar 2, 2025

Uh oh!

sakertooth commented Mar 2, 2025

szeli1 commented Nov 24, 2024 •

edited

Loading

szeli1 commented Nov 25, 2024 •

edited

Loading

sakertooth commented Nov 25, 2024 •

edited

Loading

szeli1 commented Nov 25, 2024 •

edited

Loading

szeli1 commented Nov 26, 2024 •

edited

Loading

szeli1 Mar 1, 2025 •

edited

Loading