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

Make Interfaces of AppModel accessible to Apps so apps can interact with it #931

Closed

Conversation

FrankBakkerNl
Copy link
Contributor

Breaking change

Proposed change

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
  • The code compiles without warnings (code quality chek)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

_applications.Add(app);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea probably a good idea to separate the activation and initialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most important reason was so that all Application objects (not the actual instances) are now created before the first instance is created. Before I did this, in the constructor of the first app the second was not in the list of apps yet

/// </summary>
ApplicationState State { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not want to expose the state as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am still not sure about what states we should have exactly. I was also playing with two booleans, Enabled and IsRunning. But not sure yet. For the state manager actually only enabled and disabled seem to be relevant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

aight, seem like somthing we could add in another PR since this has not been public yet

}

[NetDaemonApp]
[Focus]
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, will clean up the debug project before merge. It seems like it always has the test code for the most recently added feature

Copy link
Collaborator

@helto4real helto4real left a comment

Choose a reason for hiding this comment

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

just some questions

@helto4real helto4real deleted the branch net-daemon:dev November 3, 2023 08:39
@helto4real helto4real closed this Nov 3, 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