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

Add a default value for w.size in getWPixelSize #1233

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sambaneko
Copy link
Contributor

getWPixelSize throws the error "Uncaught (in promise) TypeError: e.size is undefined" upon Webamp initialization if the config option 'windowLayout' is used but doesn't specify all windows, or lacks a 'size' property on some windows.

So for example, either of these windowLayout values will trigger the issue:

// equalizer not specified
windowLayout: {
	main: { position: { left: 0, top: 0 } },
	playlist: {
	  position: { left: 0, top: 232 },
	  size: { extraHeight: 4, extraWidth: 0 },
	}
}

// playlist lacks 'size'
windowLayout: {
	main: { position: { left: 0, top: 0 } },
	equalizer: { position: { left: 0, top: 116 } },
	playlist: {
	  position: { left: 0, top: 232 }
	}
}

My solution was to default 'size' to [0,0] if not present, which seems to work fine (error is resolved and windows render as expected), but not sure if this would be your preferred fix. The problem windows are designated 'open: false' so that might be checked somewhere, alternatively.

@captbaritone
Copy link
Owner

Instead of setting the default in the getter, can you apply the default to the passed in config? I think the types in the rest of Webamp assume the size property is set.

Maybe also update the config types and docs to indicate that size is now optional?

@netlify
Copy link

netlify bot commented Oct 11, 2023

Deploy Preview for vigorous-lalande-5190c2 ready!

Name Link
🔨 Latest commit 9227f1d
🔍 Latest deploy log https://app.netlify.com/sites/vigorous-lalande-5190c2/deploys/65261cb7e56ecb0008b50643
😎 Deploy Preview https://deploy-preview-1233--vigorous-lalande-5190c2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sambaneko
Copy link
Contributor Author

Alternatively, I tried short circuiting the getter if window isn't open:

function getWPixelSize(w: WebampWindow, doubled: boolean) {
	if (!w.open) {
		return {height: 0, width: 0}
	}
	...

Doing that seems fine with omitting the size property; I think my previous testing was - in both cases - throwing the error only because of unspecified (thus, not open) windows (the second example leaves out the milkdrop window).

@captbaritone
Copy link
Owner

I really don't like applying the default here. The types say this value will always have a size property. I believe everywhere within Webamp the types are checked and therefore safe. The only place we can end up with a missing size is if the user passes us a window with a missing size. I think it's fine to support that, but we should do so at the boundary where the user provides the incomplete window object.

Alternatively we can validate that input object better and throw if the size is missing.

@sambaneko
Copy link
Contributor Author

I'd say this PR can be closed, but wanting to clarify that my original description of the issue is wrong. User passing a window with missing size is working fine; it's already applying default values in that case.

The issue may be specifically related to the milkdrop window, as this for example throws the error:

windowLayout: {
	main: { position: { left: 0, top: 0 } },
	equalizer: { position: { left: 0, top: 116 } },
	playlist: { position: { left: 0, top: 232 } }
}

Checking the window objects going through getWPixelSize() shows main, eq and playlist as expected, followed by a fourth window object that has only a single property, open, which is false. Error is then thrown as the size property is undefined.

If milkdrop is in there, or if another window is omitted, no error is thrown.

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