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

Rubberband fix for selecting large area in Songeditor #5003

Merged
merged 20 commits into from
Oct 7, 2019

Conversation

BaraMGB
Copy link
Contributor

@BaraMGB BaraMGB commented Jun 4, 2019

fixes #4974

scrolling and zooming while selecting an area works now. But with 180 additions and 70 deletions this is a big bug fix. I don't know if this should be merged so close before releasing 1.2.

@BaraMGB BaraMGB added the needs code review A functional code review is currently required for this PR label Jun 5, 2019
@BaraMGB BaraMGB requested a review from PhysSong June 5, 2019 18:05
@PhysSong
Copy link
Member

PhysSong commented Jun 6, 2019

I don't know if this should be merged so close before releasing 1.2.

If you want to test more, we can include this in 1.2.1 instead of 1.2.0.

Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

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

This is the first part of my review. I will review this PR again in a few days.

src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 6, 2019

Thank you for your time.

@PhysSong PhysSong added this to the 1.2.1 milestone Jun 8, 2019
include/Track.h Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
@BaraMGB BaraMGB requested a review from Reflexe June 10, 2019 13:30
@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 12, 2019

@Reflexe Thanks a lot for reviewing this.
@PhysSong Do you have any objection about this PR?

Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

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

Here are minor comments regarding whitespaces. I'm testing the functionality and will let you know if there are any issues or not.

src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
@PhysSong
Copy link
Member

I found that lassoing with Shift+drag on the timeline is broken. @BaraMGB Could you look into it?

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 12, 2019

@PhysSong Thank you. I'll look into it. 👍

@PhysSong PhysSong self-requested a review June 13, 2019 13:28
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Show resolved Hide resolved
@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 21, 2019

@PhysSong can this be merged or do you want to test it more?

include/SongEditor.h Outdated Show resolved Hide resolved
src/gui/editors/SongEditor.cpp Outdated Show resolved Hide resolved
@PhysSong
Copy link
Member

The functionality looks okay, but I think we have some time to test this more because this PR targets the stable branch.

@BaraMGB BaraMGB added needs testing This pull request needs more testing and removed needs code review A functional code review is currently required for this PR labels Jun 23, 2019
@BaraMGB
Copy link
Contributor Author

BaraMGB commented Oct 6, 2019

@PhysSong Do you think, this should go into 1.2.1?

@PhysSong
Copy link
Member

PhysSong commented Oct 6, 2019

I think it's okay to merge this now. As far as I tested, this won't bring any serious regressions.

@BaraMGB BaraMGB merged commit 5aa8788 into LMMS:stable-1.2 Oct 7, 2019
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants