Refactor AppPlugin architecture and remove legacy imviewer plugins path#73
Refactor AppPlugin architecture and remove legacy imviewer plugins path#73ehennestad wants to merge 19 commits intodevfrom
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| function optionsEditor = openOptionsEditor(obj) | ||
| %openOptionsEditor Open the ui dialog for editing method options. | ||
| titleStr = sprintf('Options Editor (%s)', obj.Name); |
Check warning
Code scanning / Code Analyzer
'Name' is referenced but is not a property, method, or event name defined in this class. Warning
| function optionsEditor = openOptionsEditor(obj) | ||
| %openOptionsEditor Open the ui dialog for editing method options. | ||
| titleStr = sprintf('Options Editor (%s)', obj.Name); | ||
| if ~isempty(obj.OptionsManager) |
Check warning
Code scanning / Code Analyzer
'OptionsManager' is referenced but is not a property, method, or event name defined in this class. Warning
| %openOptionsEditor Open the ui dialog for editing method options. | ||
| titleStr = sprintf('Options Editor (%s)', obj.Name); | ||
| if ~isempty(obj.OptionsManager) | ||
| optionsEditor = obj.OptionsManager.openOptionsEditor([], obj.getCurrentOptions()); |
Check warning
Code scanning / Code Analyzer
'OptionsManager' is referenced but is not a property, method, or event name defined in this class. Warning
| obj.hOptionsEditor = []; | ||
| obj.onOptionsEditorClosed() | ||
| if ~obj.wasAborted && obj.RunMethodOnFinish | ||
| obj.run() |
Check warning
Code scanning / Code Analyzer
'run' is referenced but is not a property, method, or event name defined in this class. Warning
| %relocatePrimaryApp Shift app and plugin windows so they do not overlap. | ||
| if nargin < 3; direction = 'horizontal'; end | ||
| figPos(1,:) = getpixelposition(hPlugin.Figure); | ||
| figPos(2,:) = getpixelposition(obj.PrimaryApp.Figure); |
Check warning
Code scanning / Code Analyzer
'PrimaryApp' is referenced but is not a property, method, or event name defined in this class. Warning
| if nargin < 3; direction = 'horizontal'; end | ||
| figPos(1,:) = getpixelposition(hPlugin.Figure); | ||
| figPos(2,:) = getpixelposition(obj.PrimaryApp.Figure); | ||
| hFig = [hPlugin.Figure, obj.PrimaryApp.Figure]; |
Check warning
Code scanning / Code Analyzer
'PrimaryApp' is referenced but is not a property, method, or event name defined in this class. Warning
| case 'horizontal'; figPos_ = figPos(:, [1,3]); dim = 1; | ||
| case 'vertical'; figPos_ = figPos(:, [2,4]); dim = 2; | ||
| end | ||
| screenPos = uim.utility.getCurrentScreenSize(obj.PrimaryApp.Figure); |
Check warning
Code scanning / Code Analyzer
'PrimaryApp' is referenced but is not a property, method, or event name defined in this class. Warning
| function options = get.Options(obj) | ||
| if ~isempty(obj.Options_) | ||
| options = obj.Options_; | ||
| elseif ~isempty(obj.OptionsManager) |
Check warning
Code scanning / Code Analyzer
'OptionsManager' is referenced but is not a property, method, or event name defined in this class. Warning
| if ~isempty(obj.Options_) | ||
| options = obj.Options_; | ||
| elseif ~isempty(obj.OptionsManager) | ||
| options = obj.OptionsManager.Options; |
Check warning
Code scanning / Code Analyzer
'OptionsManager' is referenced but is not a property, method, or event name defined in this class. Warning
Manual checklist1.
|
There was a problem hiding this comment.
Pull request overview
Refactors the plugin architecture across imviewer/applify to remove legacy plugin-path assumptions, centralize key/key-release dispatch, and migrate plugins from settings-based workflows to options-oriented and modal preview/controller patterns.
Changes:
- Introduces
ModalMethodPreviewController,HasOptionsManager, andAppBridgePluginto separate “method options” from persistent user settings and standardize modal edit/preview/run flows. - Moves
PointerManagerto be app-owned inimviewer.Appand routes key press/release events throughAppWithPlugin(including key-release dispatch). - Migrates multiple plugins (e.g., NoRMCorre/FlowRegistration/EXTRACT/FluFinder, signalviewer plugins) to the new options APIs and updates RoiClassifier to bridge/controller semantics.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| code/wrappers/+nansen/+plugin/+signalviewer/DffExplorer.m | Migrates signalviewer plugin from settings to HasOptionsManager + options callbacks. |
| code/wrappers/+nansen/+plugin/+signalviewer/CaimanDeconvolution.m | Migrates signalviewer plugin from settings to HasOptionsManager + options callbacks. |
| code/wrappers/+nansen/+plugin/+imviewer/NoRMCorre.m | Moves to modal options workflow + Options-based motion-correction preview integration. |
| code/wrappers/+nansen/+plugin/+imviewer/FluFinder.m | Moves to modal options workflow + Options-based preview updates. |
| code/wrappers/+nansen/+plugin/+imviewer/FlowRegistration.m | Moves to modal options workflow + Options-based motion-correction preview integration. |
| code/wrappers/+nansen/+plugin/+imviewer/EXTRACT.m | Moves to modal options workflow + Options-based preview updates. |
| code/graphics/+applify/AppWithPlugin.m | Adds plugin key-release event dispatch (sendKeyReleaseEventToPlugins). |
| code/graphics/+applify/+mixin/UserSettings.m | Simplifies settings editor logic (removes plugin aggregation from mixin). |
| code/graphics/+applify/+mixin/ModalMethodPreviewController.m | New behavioral mixin implementing modal edit/preview/run/destroy lifecycle. |
| code/graphics/+applify/+mixin/HasOptionsManager.m | New mixin providing per-instance OptionsManager and editOptions workflow. |
| code/graphics/+applify/+mixin/AppPlugin.m | Cleans base plugin contract (no longer inherits UserSettings; options dispatch reworked). |
| code/graphics/+applify/+mixin/AppBridgePlugin.m | New base for bridge/controller plugins that don’t own an options workflow. |
| code/apps/+imviewer/@ImviewerPlugin/ImviewerPlugin.m | Updates imviewer plugin base to rely on activation lifecycle rather than constructor wiring. |
| code/apps/+imviewer/@App/App.m | Makes PointerManager app-owned; routes key press/release through pointer manager then plugins; updates settings aggregation in app. |
| code/apps/+imviewer/+plugin/@RoiManager/RoiManager.m | Makes RoiManager a true settings owner (UserSettings) and uses app-owned PointerManager; opens RoiClassifier via openPlugin. |
| code/apps/+imviewer/+plugin/@RoiClassifier/getDefaultSettings.m | Removes legacy settings defaults for RoiClassifier (bridge plugin now). |
| code/apps/+imviewer/+plugin/@RoiClassifier/RoiClassifier.m | Migrates RoiClassifier to AppBridgePlugin and adds lifecycle cleanup hooks. |
| code/+nansen/+processing/DataMethod.m | Updates preview result harvesting from settings to Options. |
| code/+nansen/+processing/@MotionCorrectionPreview/MotionCorrectionPreview.m | Renames preview API from settings to Options and updates helper methods accordingly. |
| .github/badges/code_issues.svg | Updates badge count asset. |
Comments suppressed due to low confidence (1)
code/wrappers/+nansen/+plugin/+imviewer/NoRMCorre.m:154
- The type check
if ~isa(Y,'single') || ~isa(Y,'double')is always true (a value can’t be both single and double), soYwill always be cast to single. If the intention is “cast unless already single/double”, this should use&&(or a~(isa(...,'single') || isa(...,'double'))pattern).
if ~isa(Y, 'single') || ~isa(Y, 'double')
Y = single(Y);
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| containsOpts = isa(cellOfArgs{1}, 'struct') || ... | ||
| isa(cellOfArgs{1}, 'nansen.manage.OptionsManager'); | ||
|
|
||
| if containsOpts | ||
| opts = cellOfArgs{1}; cellOfArgs(1) = []; | ||
| end |
There was a problem hiding this comment.
optionsCheck treats an empty struct argument as “options” and strips it into opts, which then triggers assignOptions in the constructor. Because imviewer.App.openPlugin passes struct.empty by default when no options are provided, this will either (a) call assignOptionsStruct(struct.empty) and error for plugins that don't expose an Options property (e.g. AppBridgePlugin), or (b) remove the arg and leave opts empty but still change constructor behavior unintentionally. Consider explicitly treating struct.empty as “no options” (strip it from args but keep opts = []) so plugins without options can be opened safely.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| properties | ||
| OptionsManager nansen.manage.OptionsManager | ||
| end | ||
|
|
There was a problem hiding this comment.
HasOptionsManager defines an OptionsManager property, but applify.mixin.AppPlugin also defines OptionsManager. Any plugin inheriting both (e.g. AppPlugin & HasOptionsManager) will hit a duplicate/ambiguous property name error at class load time. Consider removing/renaming this property in the mixin (and rely on AppPlugin.OptionsManager), or remove OptionsManager from AppPlugin and standardize options ownership in one place.
| properties | |
| OptionsManager nansen.manage.OptionsManager | |
| end |
| function addPreviewOptions(obj) | ||
|
|
||
| S = struct(); | ||
| S.Show = 'Preprocessed'; | ||
| S.Show_ = {'Preprocessed', 'Binarized'}; | ||
|
|
||
| obj.settings_.Preview = S; | ||
| obj.Options_.Preview = S; | ||
|
|
||
| end |
There was a problem hiding this comment.
addPreviewOptions writes into obj.Options_.Preview, but if Options_ is still struct.empty (the default), this remains a 0x0 struct and get.Options will treat it as empty, so Preview never becomes visible through obj.Options. Also, this class doesn't define assignDefaultOptions, so obj.Options may be empty unless the caller passes an options struct. Initialize options to a scalar struct (or create an OptionsManager + seed Options) before adding preview options, so later obj.Options.General/Preprocessing/... accesses don't error.
| obj.relocatePrimaryApp(optionsEditor) | ||
| obj.hOptionsEditor = optionsEditor; | ||
| addlistener(obj, 'ObjectBeingDestroyed', @(s,e) delete(optionsEditor)); | ||
| end |
There was a problem hiding this comment.
The ObjectBeingDestroyed listener always calls delete(optionsEditor) (line 72). In the normal modal flow onOptionsEditorResumed already deletes obj.hOptionsEditor, and then DestroyOnFinish deletes obj, which can trigger this listener and attempt to delete an already-deleted editor handle (often throws “Invalid or deleted object”). Guard the delete with isempty/isvalid or clear the listener before deleting the editor.
|
|
||
| function openControlPanel(obj, mode) | ||
| obj.plotGrid() | ||
| obj.editSettings() | ||
| end | ||
|
|
||
| function loadSettings(~) | ||
| % This class does not have to load settings | ||
| end | ||
| function saveSettings(~) | ||
| % This class does not have to save settings | ||
| obj.editOptions() | ||
| end |
There was a problem hiding this comment.
openControlPanel calls plotGrid() before any default Options are guaranteed to exist. Since this class doesn't define assignDefaultOptions, obj.Options can be struct.empty when the plugin is opened without explicit options, and plotGrid will then error when it accesses obj.Options.Main.*. Add an assignDefaultOptions implementation (e.g., create an OptionsManager and seed Options) or ensure Options_ is initialized before calling plotGrid.
ehennestad
force-pushed
the
dev-refactor-app-plugin
branch
2 times, most recently
from
9d4f5ce to
af0d748
Compare
- Add explicit PointerManager property (accessible to ImviewerPlugin subclasses) - Add initialisePointerManager private method; call it on init and figure recreation - Replace all obj.plugins lookups for pointerManager with direct obj.PointerManager access - Remove the lowercase plugins property (no remaining call sites) - Update RoiManager to fetch PointerManager from hViewer.PointerManager directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…(Phase 2+3) - Add sendKeyReleaseEventToPlugins to AppWithPlugin, parallel to sendKeyEventToPlugins - Call sendKeyReleaseEventToPlugins in imviewer.App.onKeyReleased so plugins receive key release events through the new path - Remove legacy plugins struct aggregation from UserSettings.editSettings; composite settings aggregation now belongs on the app/controller side - Simplify editSettings: remove doDefault flag, fix indentation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Restores the behaviour removed from UserSettings.editSettings, now correctly using obj.Plugins (AppWithPlugin) instead of the removed lowercase plugins struct. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- New applify.mixin.HasOptionsManager: instance-level OptionsManager, dependent Options property, editOptions method, and onOptionsChanged hook for subclasses — clean separation from UserSettings - DffExplorer now inherits HasOptionsManager instead of using the settings workaround: assignDefaultOptions only sets OptionsManager, onOptionsChanged replaces onSettingsChanged, editOptions replaces editSettings in the constructor Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove PrimaryAppName abstract property (assigned by subclasses but never read by the base or AppWithPlugin) - Remove dead methods(Abstract,Static) block with commented-out getPluginIcon - Remove commented-out onPluginActivated abstract methods block - Remove stale Todo comment (resolved by HasOptionsManager) - Add concise comment explaining the class role and why Heterogeneous is needed - Make singleton constructor behaviour explicit: warn and return early instead of silently substituting a different object mid-construction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove assignDataIoModel (body was immediate return, fully dead) - Remove NumFrames dependent property (no get method, never used) - Remove redundant PrimaryApp and Axes assignments from constructor; both are already set via onPluginActivated during activatePlugin - Remove double onImviewerSet call (constructor no longer calls it directly; onPluginActivated is the single call site) - Remove stale %PointerManager comment (now a real property on App) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gin (Phase 7) - New applify.mixin.ModalMethodPreviewPlugin: owns RunMethodOnFinish, DestroyOnFinish, Modal, editSettings, openSettingsEditor, onSettingsEditorResumed, onSettingsEditorClosed, place, relocatePrimaryApp - AppPlugin loses all of the above; now contains only core plugin lifecycle: Name, PrimaryApp, PartialConstruction, key callbacks, activatePlugin, DataIoModel, OptionsManager, MenuItem, Icon - NoRMCorre, FlowRegistration, EXTRACT, FluFinder now inherit ModalMethodPreviewPlugin alongside ImviewerPlugin - Remove dead loadSettings/saveSettings no-op overrides from NoRMCorre, EXTRACT, FluFinder (USE_DEFAULT_SETTINGS=false already prevents file I/O) - Remove dead getPluginIcon static stubs from EXTRACT and FluFinder - Migrate CaimanDeconvolution to HasOptionsManager; remove settings workaround Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rename methods to drop the settings terminology: editSettings -> editOptions openSettingsEditor -> openOptionsEditor onSettingsEditorResumed -> onOptionsEditorResumed onSettingsEditorClosed -> onOptionsEditorClosed Update all overrides and call sites in NoRMCorre, FlowRegistration, EXTRACT and FluFinder. Fix superclass qualifier in NoRMCorre and FlowRegistration to reference ModalMethodPreviewPlugin directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
wasAborted should be public
Fix syntax error
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ehennestad
force-pushed
the
dev-refactor-app-plugin
branch
from
af0d748 to
0468fac
Compare
Summary
Main changes
imviewer.Appnow owns PointerManager directlyNotes