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

Bugfix URL parameters not being applied #292

Merged
merged 2 commits into from
Feb 28, 2024
Merged

Conversation

EmanH
Copy link
Contributor

@EmanH EmanH commented Feb 22, 2024

URL parameters are not being applied in connect.html or index.html. The root cause is that the getparam() function in Utitiles is effectively ignoring url parameters.

@totaam
Copy link
Collaborator

totaam commented Feb 23, 2024

Parameters should override session storage, by changing the order, this doesn't work any more. Does it?

@EmanH
Copy link
Contributor Author

EmanH commented Feb 25, 2024

@totaam Yes, it doesn't work. My assumption was that session params take precedence with URL params as the fallback for any undefined value. Either way makes no difference to me though, so long as URL params are being applied.

@totaam
Copy link
Collaborator

totaam commented Feb 26, 2024

Please keep the parameters as overrides for sessionStorage

@EmanH
Copy link
Contributor Author

EmanH commented Feb 28, 2024

@totaam Have changed so url parameters take precedence over parameters from sessionStorage

@totaam totaam merged commit 0a67007 into Xpra-org:master Feb 28, 2024
1 check passed
@totaam
Copy link
Collaborator

totaam commented Feb 28, 2024

I have made a mistake in merging this too quickly.

  • the function may return undefined when it used to return null
  • it would have been better to squash the commits

I have some fixes pending.

@totaam
Copy link
Collaborator

totaam commented Feb 28, 2024

@EmanH Please review the commit above.
I have reviewed all the other call sites - it doesn't look like they require changes.

@EmanH
Copy link
Contributor Author

EmanH commented Feb 28, 2024

@totaam Technically == does a fuzzy compare (in JS), so in effect:

  • null == undefined = true
  • null === undefined = false

So, these are equivalent:

if (v == null && prop in default_settings) {}
if (v == undefined && prop in default_settings) {}

The other change from if (v === null) { to if (!v) {. Is probably ok, but also fuzzy compare. So !v would pass if v was undefined, null, false or '' (empty string).

@EmanH
Copy link
Contributor Author

EmanH commented Feb 28, 2024

When assigning values, I think its best practice to consider undefined as 'no value' and consider null as 'a value'.

@totaam
Copy link
Collaborator

totaam commented Feb 29, 2024

@EmanH I know the difference.
The ones in stristrue and getboolparam were definitely bugs caused by this change as if (v === null) would no longer match.
The other in getparam was changed for consistency.
Using !v is just a shortcut, avoids converting null, undefined, empty-strings, False-booleans, etc.

totaam added a commit that referenced this pull request Mar 29, 2024
@totaam
Copy link
Collaborator

totaam commented Mar 29, 2024

And... another fix, these changes completely broke the connect page.

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