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.

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

Track operations fixes and other stuff. #3878

Merged
merged 4 commits into from
Nov 15, 2017
Merged

Track operations fixes and other stuff. #3878

merged 4 commits into from
Nov 15, 2017

Conversation

husamalhomsi
Copy link
Member

@husamalhomsi husamalhomsi commented Oct 13, 2017

Fixes #3592 and other stuff.

Before After
image image

Changes:
• Limit the ability of moving a track to its move grip.
• Clicking the move grip of a track doesn't hide the track's mute and options buttons.
• Clicking the move grip of a track changes the move grip's image to one with white squares instead of gray.
• Changed the cursor shape that shows when moving tracks to be vertical.
• Improved the position and padding of the gear image.
• Removed the trackop_c.png and trackop_h.png files and some of their related code in the style.css file of the default theme.

Hussam Eddin Alhomsi added 2 commits October 13, 2017 15:30
@gi0e5b06
Copy link

I can only approve (as I have made the same changes in my PR, including the TODO item).

@husamalhomsi
Copy link
Member Author

Could you commit the fix for the TODO item here?

@gi0e5b06
Copy link

It's here: https://github.com/gi0e5b06/lmms/blob/master/src/core/Track.cpp
Moving is limited to the grip.
I don't remember if I changed something for the drag&drop. Anyway I think it should cover the whole area, the difference being made by the mimetype.

@gi0e5b06
Copy link

Also I like the lines 1876-1880: https://github.com/gi0e5b06/lmms/blob/86a54872b9d00fbfc338f1864a903bf4d690f962/src/core/Track.cpp#L1876

if( m_trackView->isMovingTrack() )
	{
		p.setPen(Qt::red);
		p.drawLine(r.x(),r.y(),r.x(),r.y()+r.height()-1);
	}

It draws a small red line on the left border when you're moving the track.

@husamalhomsi
Copy link
Member Author

husamalhomsi commented Oct 14, 2017

Edit: added a comparison.

I tested your suggestion.

Without line With line
image image

The line makes the grip look crammed with the white squares, and isn't enough alone.
I think the white squares on the grip are enough.

@PhysSong PhysSong requested a review from Umcaruje October 18, 2017 08:14
@tresf
Copy link
Member

tresf commented Nov 15, 2017

Note, this prevents the previous behavior of dragging a track by "any random empty part of the track label" behavior that had been working previously.

Since no one has made a UX argument against this, and it actually fixes the drag handle, merging. If you want the old way, you'll have to argue it back. From a UI perspective we really don't allow rearranging of items like this in any other part of the software except the pattern editors, which aren't UI input elements.

Although I personally prefer a "long hold, then drag" like most modern software uses, that should be filed as a new feature.

@@ -2713,6 +2710,12 @@ void TrackView::dropEvent( QDropEvent * de )
*/
void TrackView::mousePressEvent( QMouseEvent * me )
{
if( me->x()>10 ) // 10 = The width of the grip + 2 pixels to the left and right.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks horizontal track resizing. I think this check should be moved to below.

@@ -2745,7 +2748,7 @@ void TrackView::mousePressEvent( QMouseEvent * me )
{
m_action = MoveTrack;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. Fixes the issue as expected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct commit or PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct commit looks fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done via e8debf9. 👍

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Fixes a regression from PR LMMS#3878 that broke horizontal track resizing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants