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

Refactor PluginRunner to OpenUtau.Core and testable #669

Merged
merged 3 commits into from
May 14, 2023

Conversation

arkfinn
Copy link
Contributor

@arkfinn arkfinn commented Apr 26, 2023

  • Plugin logic move from ViewModel to OpenUtau.Core.
  • Create interface IPlugin.( for unit test)
  • Create unit test.

This fix might makes easier for plugins test in future.
But I know we have various concept on coding.
If more good thoughts, would you give me.

last = NotesViewModel.Selection.LastOrDefault();
}
var runner = new PluginRunner(PathManager.Inst);
runner.OnReplaceNote += ((sender, args) => {
Copy link
Owner

@stakira stakira May 6, 2023

Choose a reason for hiding this comment

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

DocManager is a Core class. Maybe you don't need OnReplaceNote and OnError events? This will avoid GC complexity around event handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
I try to more simple method.

This was for the following reasons

  1. it is the responsibility of the ViewModel to add undo/redo to plugin process.
  2. DocManager has state(undoGroup), there might be a problem with parallel test.
  3. To easily mocking at test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stakira
Sorry, I understand the risks of using EventHandler in WPF.
But using DocManager from PluginRunner makes it difficult to test.
I want to remove the use of EventHandler and keep DocManager replaceable.

How about a fix like this:

  • Change OnReplaceNote from event to member variable (delete event += from ViewModel)
  • Initialize methods use DocManager
  • Test methods that use TestMethod

example:

// for ViewModel
public PluginRunner(PathManager path, DocManager doc) {
  this.replaceMethod = (path, toAdd, toRemove) => {
    doc.doSomething()
  }
  ....
}
// for Test
public PluginRunner(PathManager path, Action replaceMethod) {
  this.replaceMethod = replaceMethod
  ....
}

@arkfinn
Copy link
Contributor Author

arkfinn commented May 11, 2023

I tried to fix it like this

@stakira stakira merged commit c1a96c9 into stakira:master May 14, 2023
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.

2 participants