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

fix: move default config to class #617

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

marcozzxx810
Copy link
Contributor

related to #610

@ImUrX
Copy link
Member

ImUrX commented Mar 1, 2023

there should be a config rewrite tbh

Copy link
Member

@ImUrX ImUrX left a comment

Choose a reason for hiding this comment

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

override the fields instead of making a constructor in kotlin please

@ButterscotchV ButterscotchV added Priority: Low Less important right now Type: Enhancement Adds or improves a feature Area: Server Related to the server labels Mar 1, 2023
@ImUrX ImUrX linked an issue Mar 2, 2023 that may be closed by this pull request
@Erimelowo
Copy link
Member

I did it like how it is because it didn't work properly otherwise. Is this tested?

@ButterscotchV ButterscotchV self-requested a review March 28, 2023 22:43
Comment on lines +7 to +28
private var portIn: Int = 39540
private var portOut: Int = 39539

var anchorHip = true

var vrmJson: String? = null

override fun getPortIn(): Int {
return portIn
}

override fun setPortIn(portIn: Int) {
this.portIn = portIn
}

override fun getPortOut(): Int {
return portOut
}

override fun setPortOut(portOut: Int) {
this.portOut = portOut
}
Copy link
Member

@ButterscotchV ButterscotchV Mar 28, 2023

Choose a reason for hiding this comment

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

Can't these getters and setters just be override properties?
https://kotlinlang.org/docs/inheritance.html#overriding-properties

Suggested change
private var portIn: Int = 39540
private var portOut: Int = 39539
var anchorHip = true
var vrmJson: String? = null
override fun getPortIn(): Int {
return portIn
}
override fun setPortIn(portIn: Int) {
this.portIn = portIn
}
override fun getPortOut(): Int {
return portOut
}
override fun setPortOut(portOut: Int) {
this.portOut = portOut
}
override var portIn: Int = 39540
override var portOut: Int = 39539
var anchorHip = true
var vrmJson: String? = null

Copy link
Member

Choose a reason for hiding this comment

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

Why does VMCConfig overrides all the same from OSCConfig... also just rewrite it all in kotlin without getters and setters.

@Erimelowo
Copy link
Member

boop

@Eirenliel
Copy link
Member

@Louka3000 resolve the conversations and conflicts, what's the boop about?

Comment on lines +7 to +28
private var portIn: Int = 39540
private var portOut: Int = 39539

var anchorHip = true

var vrmJson: String? = null

override fun getPortIn(): Int {
return portIn
}

override fun setPortIn(portIn: Int) {
this.portIn = portIn
}

override fun getPortOut(): Int {
return portOut
}

override fun setPortOut(portOut: Int) {
this.portOut = portOut
}
Copy link
Member

Choose a reason for hiding this comment

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

Why does VMCConfig overrides all the same from OSCConfig... also just rewrite it all in kotlin without getters and setters.

@@ -7,10 +7,10 @@ public class OSCConfig {
private boolean enabled = false;

// Port to receive OSC messages from
private int portIn;
private int portIn = 9002;
Copy link
Member

Choose a reason for hiding this comment

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

The class gets extended and the fields get used in children, make it protected or rewrite in kotlin and make public.

@Erimelowo
Copy link
Member

@Louka3000 resolve the conversations and conflicts, what's the boop about?

The boop is about Butter’s review not being taken care of

@ImUrX ImUrX requested a review from Erimelowo as a code owner June 13, 2024 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Server Related to the server Priority: Low Less important right now Type: Enhancement Adds or improves a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defaults should not be initialized by VRConfig
5 participants