Skip to content

Fix #32261: Add zoom orientation for list type selections#33135

Open
shrutiba1 wants to merge 1 commit intomusescore:masterfrom
shrutiba1:32261-zoomorientation
Open

Fix #32261: Add zoom orientation for list type selections#33135
shrutiba1 wants to merge 1 commit intomusescore:masterfrom
shrutiba1:32261-zoomorientation

Conversation

@shrutiba1
Copy link
Copy Markdown

Resolves: #32261

The zoom orientation for list selections didn't have defined behavior, and was defaulting to the top left corner. Fixed by uniting positions of individual objects within a selection rather than treating them as a range.

We performed manual testing, as automatic testing would have required mouse/keyboard inputs that were difficult to automate. We've attached the video of our manual testing below:

Zoom.Orientation.Bug.Fix.mov
  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

Co-authored-by: Serenity Metzger <serenmet@umich.edu>
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b9a8d604-c025-462a-9b4c-68197e5de67c

📥 Commits

Reviewing files that changed from the base of the PR and between 1e84026 and 66771cb.

📒 Files selected for processing (1)
  • src/notationscene/qml/MuseScore/NotationScene/notationviewinputcontroller.cpp

📝 Walkthrough

Walkthrough

The findZoomFocusPoint() function in notationviewinputcontroller.cpp was modified to handle list selections differently. For list selections, the function now computes a union of the canvasBoundingRect() of all selected elements, converts the center of this union from logical to view coordinates, and uses it as the zoom focus point instead of applying the default single selection logic. Minor whitespace cleanup was also performed.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the fix: it addresses issue #32261 by adding zoom orientation for list type selections.
Description check ✅ Passed The PR description covers the issue, solution, testing approach, and all required checklist items are marked complete with relevant supporting evidence (manual testing video).
Linked Issues check ✅ Passed The changes directly address issue #32261 by modifying findZoomFocusPoint() to handle list selections correctly by uniting bounding rectangles instead of defaulting to top-left corner.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to fix list selection zoom behavior; only one file modified with zoom focus logic and minor whitespace cleanup, directly addressing the linked issue.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@mathesoncalum mathesoncalum left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution - looks good! I've left a comment with a requested change.

The only other thought I had was about efficiency since we're re-evaluating the centre point every time we call Cmd++ (even if the selection didn't change between calls). Ultimately I came to the conclusion that this isn't really a big deal as it would only be noticeable in absolutely enormous selections (in which case you're unlikely to be doing any zooming). Also any caching solution we implement is probably going to be more hassle than it's worth.

resultY = result.y();
} else {
// Selection: zoom at the center of the selection
PointF result = m_view->fromLogical(selection->canvasBoundingRect().center());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think NotationSelection::canvasBoundingRect would actually be the most coherent place for your newly added LIST logic. After a quick search it looks like findZoomFocusPoint is the only place we use this method (you may want to double check that), so I think we're safe to do so.

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.

Orienting zoom towards the selection doesn't work for list selections

3 participants