10223 port filter curve eq#10892
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (48)
📝 WalkthroughWalkthroughAdds a new shared axis utility module (types, scale, ticks, labels, NumberScale), introduces a reusable GridPlot QML component, implements a new Filter Curve EQ effect (C++ class, view-model, editable curve model), and QML views including a tooltip. Spectrogram code is migrated to use shared axis types/NumberScale and prior spectrogram ruler-tick utilities are removed. CMake build files are updated to include the shared module and a new AU_BUILD_SHARED_TESTS option; resources and integration wiring are added for the new QML/UI components. sequenceDiagram
participant User as User
participant UI as FilterCurveEqView (QML)
participant VM as FilterCurveEqViewModel
participant Model as FilterCurveModel
participant Effect as FilterCurveEq
participant Shared as shared::axis
User->>UI: drag/edit curve point / toggle scale / zoom
UI->>VM: invoke zoomIn/zoomOut / setLinFreqScale / setLabelWidth
UI->>Model: setPoint/addPoint/removePoint (via VM)
VM->>Model: reload / commit edits
Model->>Effect: syncToEnvelope / EnvelopeUpdated
VM->>Shared: xTicks() -> axisTicks + labelsForTicks
Shared-->>VM: AxisTick list + labels
UI->>UI: show FilterCurveEqTooltip(freq,gain)
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/shared/axis/axisticks.h (1)
28-29: ⚡ Quick winConsider widening
majorSteptodouble.Typing
majorStepasstd::optional<int>rules out fractional steps (e.g., 0.5, 0.1) for ranges below ~10, and also pairs poorly with the auto-computed step in the implementation (see comment inaxisticks.cpp). Adoublewould be both safer and more general for a shared axis library.src/shared/axis/axisticks.cpp (1)
71-76: 💤 Low valueDead-parameter nit:
getTicksignoresmin/max.
minandmaxare received but the only values forwarded togetTicksValuesaremax(twice via the local) andmin. Actuallyminisn't forwarded at all —getTicksValues(max, min, ...)is called inline. Thescaleparameter is also redundant oncenumberScaleis passed. Consider tightening the signature, or inlining this one-line helper at the single call site (Line 111).src/effects/builtin_collection/filtercurveeq/filtercurvemodel.cpp (1)
78-87: 💤 Low valueConsider calling
EnvelopeUpdated()incancelDrag()for consistency withflatten()andinvert().While
cancelDrag()successfully restores the point list and syncs the envelope back viasyncToEnvelope(), it does not notify downstream listeners viaEnvelopeUpdated(). This differs fromflatten()andinvert(), which both callEnvelopeUpdated()immediately aftersyncToEnvelope().The current code appears safe because:
mCurvescache inmCurvesListis only read afterEnvelopeUpdated()is called- The UI binds to
m_pointsthrough thepointsChanged()signal, whichcancelDrag()does emit- The legacy au3 code follows the same pattern (no
EnvelopeUpdated()on capture loss)However, adding the call would improve consistency and provide defensive coverage if future code introduces listeners expecting
EnvelopeUpdated()notifications on state reversions.src/shared/axis/numberscale.h (1)
16-17: 💤 Low value
AxisScale::Undefinedtreated asLinear— consider making the default constructor explicit about the invariant.The default constructor sets
mType = AxisScale::Undefinedand stores rawvalue0/value1(same path asLinear), but theIteratordereference at lines 178–181 returnsmValuefor bothUndefinedandLogarithmicwithout any Hz-back-conversion. ForLogarithmicthis is intentional (the value stored is already in Hz afterbegin()initialises it viaexp(...)), but forUndefinedthe dereference silently returns the stored raw value, which may not be what callers expect if they ever construct via the default constructor and then iterate.This is not a defect given current usage, but worth a short comment near the default constructor to document the
Undefined == Linearcontract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 82c409f0-e231-4d4d-869f-06a6de0956cd
📒 Files selected for processing (45)
CMakeLists.txtsrc/CMakeLists.txtsrc/effects/builtin_collection/CMakeLists.txtsrc/effects/builtin_collection/builtin_effects_collection.qrcsrc/effects/builtin_collection/dynamics/compressor/CompressionCurve.qmlsrc/effects/builtin_collection/dynamics/compressor/CompressorView.qmlsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cppsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.hsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqGridLines.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qmlsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.hsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.hsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.hsrc/effects/builtin_collection/graphiceq/equalizationbandsliders.cppsrc/effects/builtin_collection/internal/builtincollectionloader.cppsrc/shared/CMakeLists.txtsrc/shared/axis/axislabel.cppsrc/shared/axis/axislabel.hsrc/shared/axis/axisscale.hsrc/shared/axis/axisticks.cppsrc/shared/axis/axisticks.hsrc/shared/axis/axistypes.hsrc/shared/axis/numberscale.hsrc/shared/tests/CMakeLists.txtsrc/shared/tests/axisticks_tests.cppsrc/spectrogram/CMakeLists.txtsrc/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.hsrc/spectrogram/internal/frequencyselectioncontroller.cppsrc/spectrogram/internal/spectrogramservice.cppsrc/spectrogram/internal/spectrogramutils.cppsrc/spectrogram/internal/spectrogramutils.hsrc/spectrogram/spectrogramtypes.cppsrc/spectrogram/spectrogramtypes.hsrc/spectrogram/view/channelspectralselectionmodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.hsrc/uicomponents/au_uicomponents.qrcsrc/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qmlsrc/uicomponents/qml/Audacity/UiComponents/qmldir
💤 Files with no reviewable changes (3)
- src/spectrogram/internal/spectrogramutils.h
- src/spectrogram/internal/au3/au3spectrogramsettings.h
- src/spectrogram/internal/spectrogramutils.cpp
saintmatthieu
force-pushed
the
10223-port-filter-curve-eq
branch
from
f9638e6 to
7c42595
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cpp (1)
181-183: ⚡ Quick winUse adaptive major-step selection instead of fixed
1000inxTicks().Line 181 hardcodes
majorStep=1000, which can make tick density inconsistent across zoom/range and lin/log modes. LetaxisTicks()auto-pick by passingstd::nulloptfor better axis readability.Suggested fix
- auto allTicks = au::shared::axisTicks(loFreq(), hiFreq(), scale, m_labelWidth, m_axisWidth, 1000); + auto allTicks = au::shared::axisTicks(loFreq(), hiFreq(), scale, m_labelWidth, m_axisWidth, std::nullopt);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a9bcbabf-6491-4bc4-8fc2-c8e81bfc59d9
📒 Files selected for processing (31)
CMakeLists.txtsrc/CMakeLists.txtsrc/effects/builtin_collection/dynamics/compressor/CompressionCurve.qmlsrc/effects/builtin_collection/dynamics/compressor/CompressorView.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qmlsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.hsrc/shared/CMakeLists.txtsrc/shared/axis/axislabel.cppsrc/shared/axis/axislabel.hsrc/shared/axis/axisscale.hsrc/shared/axis/axisticks.cppsrc/shared/axis/axisticks.hsrc/shared/axis/axistypes.hsrc/shared/axis/numberscale.hsrc/shared/tests/CMakeLists.txtsrc/shared/tests/axisticks_tests.cppsrc/spectrogram/CMakeLists.txtsrc/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.hsrc/spectrogram/internal/frequencyselectioncontroller.cppsrc/spectrogram/internal/spectrogramservice.cppsrc/spectrogram/internal/spectrogramutils.cppsrc/spectrogram/internal/spectrogramutils.hsrc/spectrogram/spectrogramtypes.cppsrc/spectrogram/spectrogramtypes.hsrc/spectrogram/view/channelspectralselectionmodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.hsrc/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qml
💤 Files with no reviewable changes (3)
- src/spectrogram/internal/au3/au3spectrogramsettings.h
- src/spectrogram/internal/spectrogramutils.h
- src/spectrogram/internal/spectrogramutils.cpp
✅ Files skipped from review due to trivial changes (13)
- src/CMakeLists.txt
- src/spectrogram/internal/au3/au3spectrogramsettings.cpp
- src/spectrogram/spectrogramtypes.cpp
- src/spectrogram/internal/spectrogramservice.cpp
- src/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cpp
- src/shared/axis/axisscale.h
- src/spectrogram/internal/frequencyselectioncontroller.cpp
- src/spectrogram/view/channelspectralselectionmodel.cpp
- src/shared/tests/CMakeLists.txt
- src/shared/axis/axistypes.h
- src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.h
- src/shared/CMakeLists.txt
- src/spectrogram/view/spectrogramchannelrulermodel.h
🚧 Files skipped from review as they are similar to previous changes (12)
- src/spectrogram/CMakeLists.txt
- CMakeLists.txt
- src/effects/builtin_collection/dynamics/compressor/CompressorView.qml
- src/spectrogram/spectrogramtypes.h
- src/shared/axis/axislabel.h
- src/shared/tests/axisticks_tests.cpp
- src/spectrogram/view/spectrogramchannelrulermodel.cpp
- src/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qml
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qml
- src/shared/axis/axislabel.cpp
- src/shared/axis/numberscale.h
- src/effects/builtin_collection/dynamics/compressor/CompressionCurve.qml
saintmatthieu
force-pushed
the
10223-port-filter-curve-eq
branch
from
f9c2aac to
c3fb86e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/shared/tests/axisticks_tests.cpp (1)
72-80: ⚡ Quick winAdd a regression case for the clamped-log boundary (
safeMin == max).Current tests don’t cover the boundary that can produce 1–2 major ticks and stress edge-pruning logic.
Suggested test addition
TEST(axisTicks, normalized_scales_are_supported) { @@ EXPECT_GE(ticks.major.size(), kMinMajorTicks); } + +TEST(axisTicks, logarithmic_clamp_boundary_is_safe) +{ + constexpr auto min = 0.0; + constexpr auto max = 1e-6; // safeMin clamps to max + constexpr auto scale = AxisScale::Logarithmic; + constexpr auto labelExtent = 20.0; + constexpr auto axisLength = 10.0; // forces aggressive edge-overlap pruning + + const AxisTicks ticks = axisTicks(min, max, scale, labelExtent, axisLength); + EXPECT_FALSE(ticks.major.empty()); +}src/effects/builtin_collection/graphiceq/equalizationbandsliders.cpp (1)
22-22: 💤 Low valueConsider using explicit namespace qualification instead of file-scope
using namespace.The namespace directive
using namespace au::effects;is only used foreq_common::envLogToLinandeq_common::envLinToLog(lines 96, 101). Explicit qualification (e.g.,au::effects::eq_common::envLogToLin) would make the origin of these functions clearer and avoid potential name collisions if this file grows.♻️ Proposed refactor to use explicit qualification
-using namespace au::effects; - `#include` "log.h"Then update the call sites:
- eq_common::envLogToLin(m_curvesList.mParameters); + au::effects::eq_common::envLogToLin(m_curvesList.mParameters);- if (eq_common::envLinToLog(m_curvesList.mParameters)) { + if (au::effects::eq_common::envLinToLog(m_curvesList.mParameters)) {src/effects/builtin_collection/filtercurveeq/FilterCurveEqGridLines.qml (1)
75-79: 💤 Low valueConsider using
Qt.font(Object.assign(...))pattern for consistency.While the current approach (binding
font.familyandfont.pixelSizeseparately) is valid, the codebase usesQt.font(Object.assign({}, ui.theme.bodyFont, { ... }))in other files for consistency and clarity. This avoids potential future issues if additional font properties need to be inherited.♻️ Proposed refactor
FontMetrics { id: fontMetrics - font.family: ui.theme.bodyFont.family - font.pixelSize: ui.theme.bodyFont.pixelSize + font: Qt.font(Object.assign({}, ui.theme.bodyFont)) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d915c8c7-e8c5-4fe6-82c9-8ae753aac1f5
📒 Files selected for processing (46)
CMakeLists.txtmusesrc/CMakeLists.txtsrc/effects/builtin_collection/CMakeLists.txtsrc/effects/builtin_collection/builtin_effects_collection.qrcsrc/effects/builtin_collection/dynamics/compressor/CompressionCurve.qmlsrc/effects/builtin_collection/dynamics/compressor/CompressorView.qmlsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cppsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.hsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqGridLines.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qmlsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.hsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.hsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.hsrc/effects/builtin_collection/graphiceq/equalizationbandsliders.cppsrc/effects/builtin_collection/internal/builtincollectionloader.cppsrc/shared/CMakeLists.txtsrc/shared/axis/axislabel.cppsrc/shared/axis/axislabel.hsrc/shared/axis/axisscale.hsrc/shared/axis/axisticks.cppsrc/shared/axis/axisticks.hsrc/shared/axis/axistypes.hsrc/shared/axis/numberscale.hsrc/shared/tests/CMakeLists.txtsrc/shared/tests/axisticks_tests.cppsrc/spectrogram/CMakeLists.txtsrc/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.hsrc/spectrogram/internal/frequencyselectioncontroller.cppsrc/spectrogram/internal/spectrogramservice.cppsrc/spectrogram/internal/spectrogramutils.cppsrc/spectrogram/internal/spectrogramutils.hsrc/spectrogram/spectrogramtypes.cppsrc/spectrogram/spectrogramtypes.hsrc/spectrogram/view/channelspectralselectionmodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.hsrc/uicomponents/au_uicomponents.qrcsrc/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qmlsrc/uicomponents/qml/Audacity/UiComponents/qmldir
💤 Files with no reviewable changes (3)
- src/spectrogram/internal/spectrogramutils.h
- src/spectrogram/internal/au3/au3spectrogramsettings.h
- src/spectrogram/internal/spectrogramutils.cpp
✅ Files skipped from review due to trivial changes (8)
- src/spectrogram/view/channelspectralselectionmodel.cpp
- src/CMakeLists.txt
- src/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cpp
- src/spectrogram/spectrogramtypes.cpp
- src/uicomponents/qml/Audacity/UiComponents/qmldir
- src/spectrogram/internal/au3/au3spectrogramsettings.cpp
- src/effects/builtin_collection/CMakeLists.txt
- src/spectrogram/internal/frequencyselectioncontroller.cpp
🚧 Files skipped from review as they are similar to previous changes (27)
- src/shared/axis/axisscale.h
- src/shared/axis/axistypes.h
- src/spectrogram/internal/spectrogramservice.cpp
- src/effects/builtin_collection/builtin_effects_collection.qrc
- CMakeLists.txt
- src/shared/tests/CMakeLists.txt
- src/uicomponents/au_uicomponents.qrc
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.h
- muse
- src/shared/CMakeLists.txt
- src/shared/axis/axislabel.h
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.h
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.cpp
- src/spectrogram/spectrogramtypes.h
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qml
- src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.h
- src/shared/axis/axislabel.cpp
- src/effects/builtin_collection/internal/builtincollectionloader.cpp
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qml
- src/spectrogram/view/spectrogramchannelrulermodel.h
- src/effects/builtin_collection/dynamics/compressor/CompressorView.qml
- src/spectrogram/view/spectrogramchannelrulermodel.cpp
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cpp
- src/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qml
- src/effects/builtin_collection/dynamics/compressor/CompressionCurve.qml
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.cpp
- src/shared/axis/numberscale.h
saintmatthieu
force-pushed
the
10223-port-filter-curve-eq
branch
2 times, most recently
from
d4da778 to
c19aa70
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/shared/axis/axisticks.cpp (1)
100-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve bookend ticks when only min/max are present.
Lines 100-104 currently run with
majorTicks.size() == 2, which can delete one edge tick even though there is no interior tick to prune. Gate both checks onsize() > 2so only interior ticks are removed.Suggested fix
- if (majorTicks.size() > 1u && std::abs(majorTicks.front().position - (majorTicks.begin() + 1)->position) < extentFraction * 1.5) { + if (majorTicks.size() > 2u + && std::abs(majorTicks.front().position - majorTicks[1].position) < extentFraction * 1.5) { majorTicks.erase(majorTicks.begin() + 1); } - if (majorTicks.size() > 1u && std::abs(majorTicks.back().position - (majorTicks.end() - 2)->position) < extentFraction * 1.5) { + if (majorTicks.size() > 2u + && std::abs(majorTicks.back().position - majorTicks[majorTicks.size() - 2].position) < extentFraction * 1.5) { majorTicks.erase(majorTicks.end() - 2); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6931e3f0-2076-4c6a-bbd1-a39a8f7f3641
📒 Files selected for processing (47)
.gitignore.vscode/launch.jsonCMakeLists.txtmusesrc/CMakeLists.txtsrc/effects/builtin_collection/CMakeLists.txtsrc/effects/builtin_collection/builtin_effects_collection.qrcsrc/effects/builtin_collection/dynamics/compressor/CompressionCurve.qmlsrc/effects/builtin_collection/dynamics/compressor/CompressorView.qmlsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cppsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.hsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qmlsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.hsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.hsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.hsrc/effects/builtin_collection/graphiceq/equalizationbandsliders.cppsrc/effects/builtin_collection/internal/builtincollectionloader.cppsrc/shared/CMakeLists.txtsrc/shared/axis/axislabel.cppsrc/shared/axis/axislabel.hsrc/shared/axis/axisscale.hsrc/shared/axis/axisticks.cppsrc/shared/axis/axisticks.hsrc/shared/axis/axistypes.hsrc/shared/axis/numberscale.hsrc/shared/tests/CMakeLists.txtsrc/shared/tests/axisticks_tests.cppsrc/spectrogram/CMakeLists.txtsrc/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.hsrc/spectrogram/internal/frequencyselectioncontroller.cppsrc/spectrogram/internal/spectrogramservice.cppsrc/spectrogram/internal/spectrogramutils.cppsrc/spectrogram/internal/spectrogramutils.hsrc/spectrogram/spectrogramtypes.cppsrc/spectrogram/spectrogramtypes.hsrc/spectrogram/view/channelspectralselectionmodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.hsrc/uicomponents/au_uicomponents.qrcsrc/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qmlsrc/uicomponents/qml/Audacity/UiComponents/qmldir
💤 Files with no reviewable changes (3)
- src/spectrogram/internal/au3/au3spectrogramsettings.h
- src/spectrogram/internal/spectrogramutils.h
- src/spectrogram/internal/spectrogramutils.cpp
✅ Files skipped from review due to trivial changes (11)
- src/spectrogram/spectrogramtypes.cpp
- src/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cpp
- src/spectrogram/internal/au3/au3spectrogramsettings.cpp
- src/spectrogram/internal/frequencyselectioncontroller.cpp
- .gitignore
- src/uicomponents/au_uicomponents.qrc
- src/spectrogram/internal/spectrogramservice.cpp
- .vscode/launch.json
- src/spectrogram/view/channelspectralselectionmodel.cpp
- src/effects/builtin_collection/CMakeLists.txt
- src/shared/axis/axistypes.h
🚧 Files skipped from review as they are similar to previous changes (25)
- CMakeLists.txt
- src/spectrogram/view/spectrogramchannelrulermodel.h
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.h
- src/uicomponents/qml/Audacity/UiComponents/qmldir
- src/shared/tests/CMakeLists.txt
- src/CMakeLists.txt
- src/effects/builtin_collection/internal/builtincollectionloader.cpp
- src/spectrogram/CMakeLists.txt
- src/shared/tests/axisticks_tests.cpp
- src/shared/CMakeLists.txt
- src/spectrogram/spectrogramtypes.h
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.h
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.cpp
- muse
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.cpp
- src/spectrogram/view/spectrogramchannelrulermodel.cpp
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qml
- src/shared/axis/axisticks.h
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.h
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qml
- src/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qml
- src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.h
- src/effects/builtin_collection/dynamics/compressor/CompressionCurve.qml
- src/shared/axis/numberscale.h
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cpp
saintmatthieu
force-pushed
the
10223-port-filter-curve-eq
branch
from
c19aa70 to
be6d90c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 81afebb1-f95a-4831-a558-18f715585ec6
📒 Files selected for processing (47)
.gitignore.vscode/launch.jsonCMakeLists.txtmusesrc/CMakeLists.txtsrc/effects/builtin_collection/CMakeLists.txtsrc/effects/builtin_collection/builtin_effects_collection.qrcsrc/effects/builtin_collection/dynamics/compressor/CompressionCurve.qmlsrc/effects/builtin_collection/dynamics/compressor/CompressorView.qmlsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cppsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.hsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qmlsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.hsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.hsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.hsrc/effects/builtin_collection/graphiceq/equalizationbandsliders.cppsrc/effects/builtin_collection/internal/builtincollectionloader.cppsrc/shared/CMakeLists.txtsrc/shared/axis/axislabel.cppsrc/shared/axis/axislabel.hsrc/shared/axis/axisscale.hsrc/shared/axis/axisticks.cppsrc/shared/axis/axisticks.hsrc/shared/axis/axistypes.hsrc/shared/axis/numberscale.hsrc/shared/tests/CMakeLists.txtsrc/shared/tests/axisticks_tests.cppsrc/spectrogram/CMakeLists.txtsrc/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.hsrc/spectrogram/internal/frequencyselectioncontroller.cppsrc/spectrogram/internal/spectrogramservice.cppsrc/spectrogram/internal/spectrogramutils.cppsrc/spectrogram/internal/spectrogramutils.hsrc/spectrogram/spectrogramtypes.cppsrc/spectrogram/spectrogramtypes.hsrc/spectrogram/view/channelspectralselectionmodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.hsrc/uicomponents/au_uicomponents.qrcsrc/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qmlsrc/uicomponents/qml/Audacity/UiComponents/qmldir
💤 Files with no reviewable changes (3)
- src/spectrogram/internal/au3/au3spectrogramsettings.h
- src/spectrogram/internal/spectrogramutils.h
- src/spectrogram/internal/spectrogramutils.cpp
✅ Files skipped from review due to trivial changes (8)
- .gitignore
- src/spectrogram/internal/spectrogramservice.cpp
- src/spectrogram/internal/frequencyselectioncontroller.cpp
- src/spectrogram/internal/au3/au3spectrogramsettings.cpp
- src/effects/builtin_collection/builtin_effects_collection.qrc
- src/shared/axis/axistypes.h
- src/CMakeLists.txt
- src/spectrogram/spectrogramtypes.cpp
🚧 Files skipped from review as they are similar to previous changes (33)
- src/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cpp
- src/uicomponents/au_uicomponents.qrc
- muse
- src/spectrogram/view/spectrogramchannelrulermodel.h
- src/spectrogram/view/channelspectralselectionmodel.cpp
- src/effects/builtin_collection/CMakeLists.txt
- src/shared/tests/CMakeLists.txt
- CMakeLists.txt
- .vscode/launch.json
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.h
- src/shared/CMakeLists.txt
- src/spectrogram/spectrogramtypes.h
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.h
- src/effects/builtin_collection/internal/builtincollectionloader.cpp
- src/shared/axis/axisscale.h
- src/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qml
- src/shared/axis/axislabel.cpp
- src/effects/builtin_collection/dynamics/compressor/CompressorView.qml
- src/shared/axis/axisticks.h
- src/shared/tests/axisticks_tests.cpp
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.h
- src/effects/builtin_collection/graphiceq/equalizationbandsliders.cpp
- src/shared/axis/axislabel.h
- src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.h
- src/spectrogram/view/spectrogramchannelrulermodel.cpp
- src/spectrogram/CMakeLists.txt
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qml
- src/shared/axis/axisticks.cpp
- src/effects/builtin_collection/dynamics/compressor/CompressionCurve.qml
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.cpp
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qml
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cpp
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.cpp
saintmatthieu
force-pushed
the
10223-port-filter-curve-eq
branch
from
be6d90c to
3f646ea
Compare
| navigation.panel: root.buttonsNavigationPanel | ||
| navigation.order: invertButton.navigation.order + 1 | ||
|
|
||
| icon: IconCode.ZOOM_IN |
There was a problem hiding this comment.
zoom in and zoom out buttons need toolTipTitle for the voiceover
There was a problem hiding this comment.
Adding them.
Why would that not regard the other FlatButtons, though?
There was a problem hiding this comment.
Most buttons have text which is used for the voice over, but in this case there is only an icon so we have to help the accessibility system, an excerp from FlatButton.qml:
NavigationControl {
id: navCtrl
name: root.objectName !== "" ? root.objectName : "FlatButton"
enabled: root.enabled && root.visible
accessible.role: MUAccessible.Button
accessible.name: Boolean(root.text) ? root.text : root.toolTipTitle
accessible.description: root.toolTipDescription
accessible.visualItem: root
accessible.enabled: navCtrl.enabledThere was a problem hiding this comment.
I see - thanks.
| if (!muse::is_equal(majorValues.back(), safeMin)) { | ||
| majorValues.push_back(safeMin); | ||
| } | ||
| std::vector<AxisTick> majorTicks = toTicks(majorValues, numberScale, extentFraction); |
There was a problem hiding this comment.
now this allows ticks to be too close to each other in spectrogram view
| return values; | ||
| } | ||
|
|
||
| std::vector<shared::AxisTick> toTicks(const std::vector<double>& values, const shared::NumberScale& numberScale, double labelExtentFraction) |
| } | ||
| } | ||
|
|
||
| shared::AxisTicks shared::axisTicks(double min, double max, AxisScale scale, double labelExtent, double axisLength) |
There was a problem hiding this comment.
this function looks similar to FilterCurveEqViewModel::xTicksLog can we merge them? The logic is quite complex (and I don't think I fully understand all corner cases here), maybe it will be worthwhile to have a single implementation?
There was a problem hiding this comment.
The FilterCurveEqViewModel::xTicksLog implementation is a special case: on a log scale, the graphic distance from e.g. 10 to 20 is the same as 100 to 200 and the same as 1k to 2k, etc. So there the major ticks are 10, 100, 1000, etc, which gives the opportunity to implement major and minor ticks decade by decade, in a for loop.
Here, I have to support all scale types. For instance, I can't have major ticks at 10, 100, 1000, etc, for a linear scale, and probably this applies to other non-log scales (like the mel scale) too. So I found a mechanism that works ok-ish for everything. When we talked about the design of the spectrogram rulers a while ago, there was a consensus that an ok-ish result was acceptable, because we'd give the user other means of knowing exactly what frequency she was pointing at.
The fact that the labels are now on top of one another like you showed in your comment below isn't acceptable, though, and shouldn't be too hard to fix.
There was a problem hiding this comment.
I agree that the implementation is tricky. The unit tests help, though.
| filterCurveEq.curveModel.removePoint(index, completed) | ||
| } | ||
|
|
||
| onDragCancelled: { |
There was a problem hiding this comment.
How can a user cancel a drag? For Automation you can press Esc, but effect window closes on Esc
There was a problem hiding this comment.
Good catch!
Adding a fix to this.
There will remain a less severe bug until another small fix is made for this in the framework's PolylinePlot.
There was a problem hiding this comment.
| const double loLog = std::log10(kLoFreqRef); | ||
| const double hiLog = std::log10(hiFreq); | ||
| const double denom = hiLog - loLog; | ||
| bool changed = false; |
There was a problem hiding this comment.
It's changed by clamping?
That's code that was migrated from AU3 almost as-is. It turns out that the return value isn't used by any alive path (the other place being in the graphic EQ if the scale is linear there but in practice it never is, maybe it's some historical leftover).
|
|
||
| for (size_t i = 0; i < numPoints; ++i) { | ||
| if (when[i] * hiFreq >= kLoFreqRef) { | ||
| // Caution: on Linux, when (when * hiFreq) == 20, the log |
There was a problem hiding this comment.
it is not specifically a linux thing, most likely it will depend on libm and codegen. Most likely with clang/mingw on windows it will be the same.
kryksyh
left a comment
kryksyh
left a comment
There was a problem hiding this comment.
moving to QA, but at least VO should be addressed
kryksyh
force-pushed
the
10223-port-filter-curve-eq
branch
from
3f646ea to
9cf2829
Compare
|
rebased to fix export dialog |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cpp (1)
236-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
multiplierwhen it starts at 10 to keep decade iteration correct.
std::ceil(loFreq() / increment)can yield10, which desynchronizes the++multiplier % 10rollover logic and produces wrong/duplicated log ticks in the first decade.Suggested fix
auto exponent = std::floor(left); auto increment = std::pow(10, exponent); int multiplier = std::ceil(loFreq() / increment); + if (multiplier >= 10) { + multiplier = 1; + ++exponent; + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1960ba76-2af1-4d1b-b56b-041002dd084a
📒 Files selected for processing (47)
.gitignore.vscode/launch.jsonCMakeLists.txtmusesrc/CMakeLists.txtsrc/effects/builtin_collection/CMakeLists.txtsrc/effects/builtin_collection/builtin_effects_collection.qrcsrc/effects/builtin_collection/dynamics/compressor/CompressionCurve.qmlsrc/effects/builtin_collection/dynamics/compressor/CompressorView.qmlsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cppsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.hsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qmlsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.hsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.hsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.hsrc/effects/builtin_collection/graphiceq/equalizationbandsliders.cppsrc/effects/builtin_collection/internal/builtincollectionloader.cppsrc/shared/CMakeLists.txtsrc/shared/axis/axislabel.cppsrc/shared/axis/axislabel.hsrc/shared/axis/axisscale.hsrc/shared/axis/axisticks.cppsrc/shared/axis/axisticks.hsrc/shared/axis/axistypes.hsrc/shared/axis/numberscale.hsrc/shared/tests/CMakeLists.txtsrc/shared/tests/axisticks_tests.cppsrc/spectrogram/CMakeLists.txtsrc/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.hsrc/spectrogram/internal/frequencyselectioncontroller.cppsrc/spectrogram/internal/spectrogramservice.cppsrc/spectrogram/internal/spectrogramutils.cppsrc/spectrogram/internal/spectrogramutils.hsrc/spectrogram/spectrogramtypes.cppsrc/spectrogram/spectrogramtypes.hsrc/spectrogram/view/channelspectralselectionmodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.hsrc/uicomponents/au_uicomponents.qrcsrc/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qmlsrc/uicomponents/qml/Audacity/UiComponents/qmldir
💤 Files with no reviewable changes (3)
- src/spectrogram/internal/spectrogramutils.h
- src/spectrogram/internal/au3/au3spectrogramsettings.h
- src/spectrogram/internal/spectrogramutils.cpp
✅ Files skipped from review due to trivial changes (10)
- src/uicomponents/au_uicomponents.qrc
- muse
- src/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cpp
- src/effects/builtin_collection/builtin_effects_collection.qrc
- src/spectrogram/internal/au3/au3spectrogramsettings.cpp
- .gitignore
- src/spectrogram/spectrogramtypes.cpp
- src/spectrogram/internal/frequencyselectioncontroller.cpp
- src/spectrogram/view/spectrogramchannelrulermodel.h
- src/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (30)
- .vscode/launch.json
- src/spectrogram/internal/spectrogramservice.cpp
- CMakeLists.txt
- src/shared/tests/CMakeLists.txt
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.h
- src/shared/axis/axisscale.h
- src/spectrogram/view/channelspectralselectionmodel.cpp
- src/uicomponents/qml/Audacity/UiComponents/qmldir
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.cpp
- src/spectrogram/CMakeLists.txt
- src/effects/builtin_collection/internal/builtincollectionloader.cpp
- src/shared/axis/axistypes.h
- src/effects/builtin_collection/dynamics/compressor/CompressorView.qml
- src/shared/CMakeLists.txt
- src/shared/axis/axislabel.h
- src/effects/builtin_collection/dynamics/compressor/CompressionCurve.qml
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.h
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.h
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qml
- src/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qml
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qml
- src/effects/builtin_collection/graphiceq/equalizationbandsliders.cpp
- src/effects/builtin_collection/CMakeLists.txt
- src/spectrogram/spectrogramtypes.h
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.cpp
- src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.h
- src/shared/axis/axisticks.h
- src/shared/axis/numberscale.h
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cpp
- src/shared/axis/axislabel.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cpp (1)
236-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
xTicksLog: startingmultipliercan be 10, causing first-decade corruption.At line 238,
std::ceil(loFreq() / increment)can return10whenloFreq()lies in(9·10^e, 10^(e+1))(e.g., 95, 99.99, 950). The wrap check at lines 251-254 only triggers when++multiplier % 10 == 0. Starting at10, the sequence goes10 → 11 → … → 19 → 20, emitting ticks at incorrect decade boundaries (10·10^e, 11·10^e, ... instead of 1·10^(e+1), 2·10^(e+1), ...).While the default 20 Hz
loFreqavoids this,mParameters.mLoFreqis user/preset configurable.🛠️ Suggested fix
Normalize the initial multiplier/exponent pair immediately after
ceil:auto exponent = std::floor(left); auto increment = std::pow(10, exponent); int multiplier = std::ceil(loFreq() / increment); + if (multiplier >= 10) { + multiplier = 1; + ++exponent; + increment = std::pow(10, exponent); + }
🧹 Nitpick comments (2)
src/effects/builtin_collection/graphiceq/GraphicEqView.qml (1)
74-84: 💤 Low valueButton labeled "Reset" still calls
flatten()method.The button ID and text were changed from "Flatten" to "Reset" (lines 74, 82), but the
onClickedhandler at line 84 still callsgraphicEq.bandsModel.flatten(). If "Reset" is the preferred user-facing term, consider renaming the underlying method for consistency, or document why different terminology is used in the UI vs. the model API.src/shared/axis/axislabel.cpp (1)
17-17: 💤 Low valueRedundant parentheses in the while condition.
Line 17 has double parentheses in the
whilecondition:while (((label.contains('.') && (label.endsWith('0'))) || label.endsWith('.'))). The outer parentheses are unnecessary.♻️ Simplification
- while (((label.contains('.') && (label.endsWith('0'))) || label.endsWith('.'))) { + while ((label.contains('.') && label.endsWith('0')) || label.endsWith('.')) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5b595752-5627-4551-ba7d-b535c972634d
📒 Files selected for processing (10)
src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cppsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qmlsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cppsrc/effects/builtin_collection/graphiceq/GraphicEqView.qmlsrc/shared/axis/axislabel.cppsrc/shared/axis/axislabel.hsrc/shared/axis/axisticks.cppsrc/shared/axis/axisticks.hsrc/shared/tests/axisticks_tests.cppsrc/spectrogram/view/spectrogramchannelrulermodel.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/shared/axis/axislabel.h
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qml
saintmatthieu
force-pushed
the
10223-port-filter-curve-eq
branch
from
6b3be62 to
a5fb65c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cpp (1)
236-255:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
multipliercan start at 10, corrupting the first decade's tick pattern.When
loFreq()falls in the range (9×10^e, 10^(e+1)),std::ceil(loFreq() / increment)returns 10. The subsequent loop increments from 10 → 11 → ... → 19 → 20, wrapping only when++multiplierreaches a multiple of 10. This produces ticks at 10×10^e, 11×10^e, ..., 19×10^e (values from the next decade) before wrapping, and causes the major-tick checkmultiplier == 1 || multiplier == 5to select the wrong frequencies.With the default 20 Hz
loFreqthis doesn't trigger, butmParameters.mLoFreqis user-configurable (e.g., 95 Hz would exhibit the issue).🔧 Proposed fix
Normalize the initial multiplier/exponent pair immediately after the
ceil:auto exponent = std::floor(left); auto increment = std::pow(10, exponent); int multiplier = std::ceil(loFreq() / increment); + if (multiplier >= 10) { + // ceil() can yield 10 when loFreq is just below a power of 10. + // Normalize into [1, 9] so the wrap logic stays in sync. + multiplier = 1; + ++exponent; + }src/shared/axis/axislabel.cpp (1)
82-88:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical:
labels.clear()empties the vector before indexed access.Line 82 resizes
labelstoticks.size(), but line 85 callslabels.clear(), which empties the vector (size becomes 0). The subsequentlabels[tick.index]access at line 87 is out-of-bounds and triggers undefined behavior. The static analyzer correctly flagged this as containerOutOfBounds.🛠️ Proposed fix
Remove the
clear()call and reuse the resized vector by overwriting each element:std::vector<std::string> labels; labels.resize(ticks.size()); int numDecimalDigits = 0; while (numDecimalDigits <= maxDecimalDigits) { - labels.clear(); + // Reuse the resized vector; overwrite each relevant element for (const auto& tick : ticksWithIndex) { labels[tick.index] = valueToLabel(tick.tick.val, numDecimalDigits); }This preserves the allocated size and correctly updates each label slot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 108778a1-8c6c-442e-8515-7ab8f7e15515
📒 Files selected for processing (48)
.gitignore.vscode/launch.jsonCMakeLists.txtmusesrc/CMakeLists.txtsrc/effects/builtin_collection/CMakeLists.txtsrc/effects/builtin_collection/builtin_effects_collection.qrcsrc/effects/builtin_collection/dynamics/compressor/CompressionCurve.qmlsrc/effects/builtin_collection/dynamics/compressor/CompressorView.qmlsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cppsrc/effects/builtin_collection/equalization_common/equalizationenvelopeutils.hsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qmlsrc/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qmlsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeq.hsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.hsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.cppsrc/effects/builtin_collection/filtercurveeq/filtercurvemodel.hsrc/effects/builtin_collection/graphiceq/GraphicEqView.qmlsrc/effects/builtin_collection/graphiceq/equalizationbandsliders.cppsrc/effects/builtin_collection/internal/builtincollectionloader.cppsrc/shared/CMakeLists.txtsrc/shared/axis/axislabel.cppsrc/shared/axis/axislabel.hsrc/shared/axis/axisscale.hsrc/shared/axis/axisticks.cppsrc/shared/axis/axisticks.hsrc/shared/axis/axistypes.hsrc/shared/axis/numberscale.hsrc/shared/tests/CMakeLists.txtsrc/shared/tests/axisticks_tests.cppsrc/spectrogram/CMakeLists.txtsrc/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.cppsrc/spectrogram/internal/au3/au3spectrogramsettings.hsrc/spectrogram/internal/frequencyselectioncontroller.cppsrc/spectrogram/internal/spectrogramservice.cppsrc/spectrogram/internal/spectrogramutils.cppsrc/spectrogram/internal/spectrogramutils.hsrc/spectrogram/spectrogramtypes.cppsrc/spectrogram/spectrogramtypes.hsrc/spectrogram/view/channelspectralselectionmodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.cppsrc/spectrogram/view/spectrogramchannelrulermodel.hsrc/uicomponents/au_uicomponents.qrcsrc/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qmlsrc/uicomponents/qml/Audacity/UiComponents/qmldir
💤 Files with no reviewable changes (3)
- src/spectrogram/internal/spectrogramutils.h
- src/spectrogram/internal/spectrogramutils.cpp
- src/spectrogram/internal/au3/au3spectrogramsettings.h
✅ Files skipped from review due to trivial changes (7)
- src/spectrogram/internal/au3/au3spectrogramclipchannelpainter.cpp
- src/spectrogram/internal/au3/au3spectrogramsettings.cpp
- src/spectrogram/spectrogramtypes.cpp
- muse
- src/spectrogram/view/channelspectralselectionmodel.cpp
- src/spectrogram/internal/frequencyselectioncontroller.cpp
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (30)
- src/uicomponents/au_uicomponents.qrc
- src/spectrogram/internal/spectrogramservice.cpp
- src/effects/builtin_collection/internal/builtincollectionloader.cpp
- src/shared/axis/axisscale.h
- src/uicomponents/qml/Audacity/UiComponents/qmldir
- CMakeLists.txt
- src/shared/axis/axistypes.h
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.cpp
- src/shared/CMakeLists.txt
- src/shared/tests/axisticks_tests.cpp
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.cpp
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.h
- src/shared/tests/CMakeLists.txt
- .vscode/launch.json
- src/effects/builtin_collection/filtercurveeq/filtercurveeq.h
- src/spectrogram/CMakeLists.txt
- src/shared/axis/axislabel.h
- src/effects/builtin_collection/equalization_common/equalizationenvelopeutils.h
- src/effects/builtin_collection/dynamics/compressor/CompressionCurve.qml
- src/shared/axis/axisticks.h
- src/effects/builtin_collection/CMakeLists.txt
- src/effects/builtin_collection/filtercurveeq/filtercurvemodel.cpp
- src/effects/builtin_collection/graphiceq/equalizationbandsliders.cpp
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqTooltip.qml
- src/uicomponents/qml/Audacity/UiComponents/components/GridPlot.qml
- src/effects/builtin_collection/filtercurveeq/FilterCurveEqView.qml
- src/shared/axis/axisticks.cpp
- src/spectrogram/view/spectrogramchannelrulermodel.cpp
- src/effects/builtin_collection/filtercurveeq/filtercurveeqviewmodel.h
- src/shared/axis/numberscale.h
| while (((label.contains('.') && (label.endsWith('0'))) || label.endsWith('.'))) { | ||
| label.chop(1); | ||
| } | ||
| return label.toStdString() + "k"; |
There was a problem hiding this comment.
most likely this unit should be translated, for russian it should be к instead of k as in кГц vs kHz
Co-authored-by: Copilot <copilot@github.com>
…ntegration of GridPlot therein)
saintmatthieu
force-pushed
the
10223-port-filter-curve-eq
branch
from
a5fb65c to
6ed28ad
Compare
2 participants
Resolves: #10223
Recommended:
QA: