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

improve mouse pointer in piano roll editor #3065

Closed
wants to merge 6 commits into from

Conversation

BaraMGB
Copy link
Contributor

@BaraMGB BaraMGB commented Oct 6, 2016

I can't make screenshots with mouse pointer.

This PR change the following things:

  • the mouse pointer changes its appearance proper on all borders of the key area
  • we use a pencil ( or eraser ) instead of the arrow with pencil icon
  • the rubberband icon don't stuck anymore on a border when the mouse pointer leaves the area

Copy link
Member

@Umcaruje Umcaruje left a comment

Choose a reason for hiding this comment

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

Generally there is a lot of random identation changes all over the PR that make it hard to review. Please fix this.

@@ -216,7 +218,7 @@ PianoRoll::PianoRoll() :
m_noteEditMenu->addAction( act );
}
connect( signalMapper, SIGNAL(mapped(int)),
this, SLOT(changeNoteEditMode(int)) );
this, SLOT(changeNoteEditMode(int)) );
Copy link
Member

Choose a reason for hiding this comment

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

Why is a space added here

@@ -261,32 +263,32 @@ PianoRoll::PianoRoll() :
if( s_whiteKeySmallPm == NULL )
{
s_whiteKeySmallPm = new QPixmap( embed::getIconPixmap(
"pr_white_key_small" ) );
"pr_white_key_small" ) );
Copy link
Member

Choose a reason for hiding this comment

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

Why are all these lines more indented

Song::Mode_PlayPattern ),
m_currentPosition, this );
Engine::getSong()->getPlayPos(
Song::Mode_PlayPattern ),
Copy link
Member

Choose a reason for hiding this comment

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

and here

@Umcaruje
Copy link
Member

Umcaruje commented Oct 7, 2016

I'll now go and test this out

@Umcaruje
Copy link
Member

Umcaruje commented Oct 7, 2016

This works nice, and feels a lot more natural. However, the automation editor is now inconsistent with this, so I guess the same thing should be done there. Other than the indentation issues, this looks ok to me.

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Oct 7, 2016

Fine, I'll fix it. Perhaps this should be tested on Apple and Windows, too.

repaint();
return;
}

Copy link
Member

@Umcaruje Umcaruje Oct 8, 2016

Choose a reason for hiding this comment

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

unnecessary white space here

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Oct 8, 2016

@tresf can you test this on mac, please?

@tresf
Copy link
Member

tresf commented Oct 9, 2016

I can't make screenshots with mouse pointer.

You can with a cellphone or a virtual machine.

@tresf can you test this on mac, please?

I don't see any OS-specific code, so I'm not sure why this is needed. I have MacOS Qt4 and Qt5 builds here: https:/tresf/lmms/releases/tag/v1.2.0-RC2-MOUSE

Test results Qt5 10.8 Homebrew build running on 10.11:

  • the mouse pointer changes its appearance proper on all borders of the key area
  • we use a pencil ( or eraser ) instead of the arrow with pencil icon
    • No, the 4-directional arrow is what's displayed on Mac.
  • the rubberband icon don't stuck anymore on a border when the mouse pointer leaves the area

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Oct 9, 2016

I don't see any OS-specific code, so I'm not sure why this is needed.

Because the behavior is completely different from what I see on Linux. :(

@liushuyu
Copy link
Member

liushuyu commented Nov 8, 2016

So is this cooled down or still working in progress? I think this fix is somewhat important

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Nov 8, 2016

I need A video of what's happening in macOS. @simonvanderveldt?

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jan 27, 2017

I would like to merge this into master, but I can not test if this works on MacOS. Can someone test this?

@tresf
Copy link
Member

tresf commented Jan 27, 2017

I would like to merge this into master, but I can not test if this works on MacOS. Can someone test this?

Test what? I don't see any new commits since my testing on 2016-10-09

@zonkmachine
Copy link
Member

This works well over here.

  • No, the 4-directional arrow is what's displayed on Mac.

I had a peek at the qt bug tracker but nothing popped up as an absolute fit for this case. Though there were other reports of wrong cursors displayed.

@BaraMGB If this needs more testing you might wanna squash and rebase it.

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Feb 8, 2017

although the cursor lingers after hovering over a note.

@tresf can you test please if the Qt::SizeAllCursor is shown anymore and if it's stuck on leaving the note?

@zonkmachine
Copy link
Member

I just noticed the cursor is a bit nervous around the border to the NoteEditArea. It's a bit difficult to hit the spot where you resize and when you're doing the ActionResizeNoteEditArea the cursor is flickering.

@tresf
Copy link
Member

tresf commented Feb 9, 2017

can you test please if the Qt::SizeAllCursor is shown anymore and if it's stuck on leaving the note?

Just re-tested on fresh BaraMGB/pianorollmousepointer branch and it's still stuck on Mac. Results are identical to the animated GIFs from #3065 (comment).

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 1, 2017

I'll close this out. At the moment I have no chance to debug it on a mac. Perhaps I'll find a way in the future.

@BaraMGB BaraMGB closed this Jun 1, 2017
@zonkmachine
Copy link
Member

Maybe a hint in #3926

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.

5 participants