Skip to content

Context resolver fix#10983

Open
grliszas14 wants to merge 1 commit into
audacity:masterfrom
grliszas14:spectrogram_selection_fix
Open

Context resolver fix#10983
grliszas14 wants to merge 1 commit into
audacity:masterfrom
grliszas14:spectrogram_selection_fix

Conversation

@grliszas14
Copy link
Copy Markdown
Contributor

@grliszas14 grliszas14 commented May 21, 2026

Resolves: #10339

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

QA:

  • Testflow test cases have been run
  • In addition to main issue please test other shortcuts (open and close project multiple times, try to open different dialogs/context menu in the meantime), keyboard navigation (both cycling through panels/clips as well as moving playhead or making selections using arrows)
  • Check popups navigation

@grliszas14 grliszas14 requested a review from kryksyh May 21, 2026 11:57
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 21, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR modifies the UI context resolution logic in UiContextResolver::resolveUiContext(). When handling project page navigation, the resolver now classifies the UI context based on the active navigation section's type rather than its name. If the active section is of type Exclusive, it returns UiCtxProjectOpened; otherwise, including when no active section exists, it returns UiCtxProjectFocused. The remaining resolver behavior for home pages, diagnostics, and fallbacks is unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Context resolver fix' is vague and generic, not clearly describing the specific change made to the UI context resolution logic. Provide a more specific title that describes the actual change, such as 'Use navigation section type instead of name in UI context resolution' or 'Fix UI context classification based on navigation section type'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description follows the template with all required sections completed, including a link to the resolved issue, checklist items, and QA considerations.
Linked Issues check ✅ Passed The code change correctly addresses the root cause by refactoring UI context resolution to use navigation section types instead of name comparisons, which should fix the crash when copying/pasting after changing track views [#10339].
Out of Scope Changes check ✅ Passed All changes in uicontextresolver.cpp are directly scoped to fixing UI context resolution logic as required by issue #10339, with no unrelated modifications.

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


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.


INavigationSection* activeSection = navigationController()->activeSection();
if (activeSection) {
if (activeSection->name() == DEFAULT_NAVIGATION_SECTION) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

DEFAULT_NAVIGATION_SECTION - not used anymore

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

}

return context::UiCtxProjectOpened;
//! No navigation section holds explicit focus. This happens when a popup,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can this affect pop-up dialogs?

@grliszas14 grliszas14 force-pushed the spectrogram_selection_fix branch from 330a572 to f9f74b7 Compare May 21, 2026 13:06
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
src/context/internal/uicontextresolver.cpp (1)

91-111: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Verify modal/native dialogs always create an Exclusive navigation section.

The new fallback returns UiCtxProjectFocused whenever activeSection() is null, and the inline rationale only covers the "popup closed without restoring focus" case. If any modal dialog, popover, or context menu in Audacity is shown without first creating an INavigationSection of type Exclusive (e.g., a native OS dialog, a Qt QDialog/QMenu that bypasses muse::ui::navigation, or a transient overlay), activeSection() will be null while the dialog is on top — and CTX_PROJECT_FOCUSED shortcuts (Ctrl+C/Ctrl+V/Ctrl+X/Del/...) will now match and fire against the underlying project view.

This is also the unresolved concern raised earlier ("can this affect pop-up dialogs?"). Please confirm that every dialog/popup path on the project page funnels through an Exclusive navigation section before merge; otherwise this fix trades the original bug for shortcuts leaking into modal contexts.

Run the following to inventory how Exclusive navigation sections are created and where native/Qt dialogs may be shown without one:

#!/bin/bash
# 1) Where are Exclusive navigation sections constructed?
rg -nP --type=cpp --type=qml -C3 '\bINavigationSection::Type::Exclusive\b|\bType\s*::\s*Exclusive\b'

# 2) Where is NavigationSection(...) constructed/registered with a type argument?
rg -nP --type=cpp --type=qml -C2 '\bNavigationSection\s*\{?\s*[^;]*Exclusive'

# 3) Possible non-muse modal dialogs that may NOT register a NavigationSection.
rg -nP --type=cpp -C2 '\bQDialog\b|\bQMessageBox\b|\bQFileDialog\b|\bQMenu\b|\bexec\(\)|\bexecSync\b'

# 4) QML popups/dialogs that may sit on top of the project page.
rg -nP --type=qml -C2 '\bPopup\b|\bDialog\b|\bMenu\b|\bStyledDialogView\b'

# 5) Confirm activeSection() semantics — does Audacity's NavigationController clear active section
#    while a non-Exclusive dialog is open, or only when an Exclusive one is active?
fd -e cpp -e h navigationcontroller --exec rg -nP -C3 '\bactiveSection\b|\bsetActiveSection\b'

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 17b785c1-138e-42e4-b455-ca1d7bd0523b

📥 Commits

Reviewing files that changed from the base of the PR and between 330a572 and f9f74b7.

📒 Files selected for processing (1)
  • src/context/internal/uicontextresolver.cpp

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.

Copy/paste of Audio Clip doesnt work if Track Views were Changed

2 participants