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

Expose internal structs #8

Closed
muesli opened this issue Jun 14, 2021 · 8 comments
Closed

Expose internal structs #8

muesli opened this issue Jun 14, 2021 · 8 comments

Comments

@muesli
Copy link
Contributor

muesli commented Jun 14, 2021

Great work on this!

The only issue I'm currently facing is accessing nested structs like Font in SetTextFreetype2PropertiesParams. Since the struct isn't a proper type, it's a bit awkward to first initialize the parent in order to access the nested struct.

Do you think we could improve the generator here?

@andreykaipov
Copy link
Owner

andreykaipov commented Jun 14, 2021

Oh that was quick.

I see what you mean for the need of exposing the anonymous structs. Having to do either of these is pretty annoying:

       	params := &sources.SetTextGDIPlusPropertiesParams{}
	params.Source = "hello"
	params.Text = "abc"
	params.Font.Face = "Arial Black"
	params.Font.Size = 20
	_, _ = client.Sources.SetTextGDIPlusProperties(params)
	_, _ = client.Sources.SetTextGDIPlusProperties(&sources.SetTextGDIPlusPropertiesParams{
		Source: "hello",
		Text:   "abc",
		Font: struct {
			Face  string `json:"face"`
			Flags int    `json:"flags"`
			Size  int    `json:"size"`
			Style string `json:"style"`
		}{
			Face: "Arial Black",
			Size: 16,
		},
	})

This switch case https://github.com/andreykaipov/goobs/blob/v0.5.0/internal/comments/dotparser.go#L91-L110 is the one responsible for generating the anonymous structs, so ideally I could just lift them out and create them at the root of the returned *jen.Statement.

I have to poke around some more but yeah I think it should be possible without too much headache!

@muesli
Copy link
Contributor Author

muesli commented Jun 14, 2021

WIP: muesli/obs-cli#8

@muesli
Copy link
Contributor Author

muesli commented Jun 14, 2021

This looks really good already. The only blocker for my stuff is now #9, which could break existing obs-cli integrations due to the log.Printf call on every connection (even tho that goes to stderr if I'm not mistaken).

@andreykaipov
Copy link
Owner

So I got a working example, but there's a redeclaration issue I'm not sure how I'd like to handle yet.

For example, something like GetSceneItemProperties and SetSceneItemProperties both involve a Bounds object, but they can't both declare it since they're both part of the sources package.

I could figure out a way to only generate the first encountered object, and assume all the others are the same (which they all currently are), but I think the safer option is to call them something like GetSceneItemPropertiesBounds and SetSceneItemPropertiesBounds respectively, despite how verbose that is.

So now the above snippet would look something like this:

	_, _ = client.Sources.SetTextGDIPlusProperties(&sources.SetTextGDIPlusPropertiesParams{
		Source: "hello",
		Text:   "abc",
		Font: &sources.SetTextGDIPlusPropertiesFont{
			Face: "Arial Black",
			Size: 16,
		},
	})

What do you think?

@muesli
Copy link
Contributor Author

muesli commented Jun 15, 2021

The downside to this is that you can't pass the structs you retrieved from a Get call back to a Set call, like I currently do here:

https://github.com/muesli/obs-cli/pull/8/files#diff-b05ede2651fc94b7c80f4c5d2d19d667cf49b6f960262b7bc71a44e36b1e1fe1R37

@andreykaipov
Copy link
Owner

You're right that would be nice.

When I was looking through the protocol doc to figure this out, it made me wish Font was a given typedef, like how only OBSStats is given as the type of stats for the Heartbeat response, because then I wouldn't have to change anything!

So, I had the idea of just manually defining all the common structs (Font, Settings, Item, etc.), and finding when the generator should use them. Would introduce a good chunk of conditionals, but I think it's worth the trouble. Should be something like this afterwards:

                Font: &common.Font{
			Face: "Arial Black",
			Size: 16,
		},

@andreykaipov
Copy link
Owner

Should be good now!! I was using your obs-cli for my testing. Might make a PR to your goobs branch.

After I address #12 I'll cut a new release!

@muesli
Copy link
Contributor Author

muesli commented Jun 19, 2021

Fantastic, I'll check it out as well!

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

No branches or pull requests

2 participants