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

Server: Fix --serverinfo country code misinterpretation on Qt6 #2829

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

hoffie
Copy link
Member

@hoffie hoffie commented Aug 30, 2022

Short description of changes

  • Util: Make CLocale::IsCountryCodeSupported more robust

    On Qt6, we previously relied on the caller to only supply country codes within bounds. With this change we properly check before risking an uninitialized array access.

    On Qt5, we did not do any checks at all. Now we ensure that we what is provided is a valid Qt5 country code.

  • Server: Fix --serverinfo country code misinterpretation on Qt6

    Previously, country codes in --serverinfo were interpreted natively.
    This worked for Qt5, but caused unintended changes on Qt6 builds as the codes differ. Not doing a conversion for --serverinfo was an oversight from the initial Qt6 compatibility work, which is now fixed with this change.

CHANGELOG: Server: Fixed --serverinfo country code misinterpretation introduced in Jamulus 3.9.0 on Qt6-based builds such as Mac

Context: Fixes an issue?

Related: #2299
Fixes: #2809

Does this change need documentation? What needs to be documented and how?

Status of this Pull Request

Ready. Tested both on Qt5 and Qt6. Qt6 builds with this change behave like Qt5 builds again.

What is missing until this pull request can be merged?
Reviews.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie hoffie added this to the Release 3.9.1 milestone Aug 30, 2022
@hoffie hoffie added this to Triage in Tracking (old) via automation Aug 30, 2022
@hoffie
Copy link
Member Author

hoffie commented Aug 30, 2022

@acerix Could you please try a build from this PR and confirm if it works as expected with your original, pre-3.9.0 --serverinfo line?

@hoffie hoffie marked this pull request as draft August 30, 2022 21:30
src/util.cpp Show resolved Hide resolved
@acerix
Copy link

acerix commented Aug 31, 2022

@acerix Could you please try a build from this PR and confirm if it works as expected with your original, pre-3.9.0 --serverinfo line?

I can confirm this PR reverts back to my previous "38: Canada" setting with 3.8 and Qt5.

Thanks @hoffie for the astounding support.

@hoffie hoffie requested a review from pljones August 31, 2022 19:39
src/util.cpp Show resolved Hide resolved
@pljones pljones moved this from Triage to In Progress in Tracking (old) Sep 3, 2022
@pljones pljones moved this from In Progress to Waiting on Team in Tracking (old) Sep 3, 2022
@pljones
Copy link
Collaborator

pljones commented Sep 7, 2022

@hoffie can you pick this up?

@pljones pljones added the bug Something isn't working label Sep 7, 2022
On Qt6, we previously relied on the caller to only supply country codes within
bounds. With this change we properly check before risking an uninitialized array
access.

On Qt5, we did not do any checks at all. Now we ensure that we what is
provided is a valid Qt5 country code.

Related: jamulussoftware#2809
Previously, country codes in --serverinfo were interpreted natively.
This worked for Qt5, but caused unintended changes on Qt6 builds as the
codes differ. Not doing a conversion for --serverinfo was an oversight
from the initial Qt6 compatibility work, which is now fixed with this
change.

Related: jamulussoftware#2299
Fixes: jamulussoftware#2809
@hoffie hoffie requested a review from ann0see September 10, 2022 05:45
@ann0see ann0see added the needs documentation PRs requiring documentation changes or additions label Sep 11, 2022
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Tested on macOS and it's ok. Thanks!

Tracking (old) automation moved this from Waiting on Team to In Progress Sep 11, 2022
@hoffie hoffie merged commit 0fbbaff into jamulussoftware:master Sep 12, 2022
Tracking (old) automation moved this from In Progress to Done Sep 12, 2022
@hoffie
Copy link
Member Author

hoffie commented Sep 12, 2022

Removing needs documentation tag as the reminder has been added to #2813.

@hoffie hoffie deleted the fix-serverinfo-country-qt6 branch September 12, 2022 20:39
@hoffie hoffie removed the needs documentation PRs requiring documentation changes or additions label Sep 12, 2022
@pljones
Copy link
Collaborator

pljones commented Sep 12, 2022

Removing needs documentation tag as the reminder has been added to #2813.

Can you get an issue raised, too, please (and added to the board).

@hoffie
Copy link
Member Author

hoffie commented Sep 12, 2022

Can you get an issue raised, too, please (and added to the board).

Hm? We have collected all pre-known release announcement items right in the existing 3.9.1 tracker #2813. I have added this PR there (that's what I meant above). Do you want a separate issue for the sub-task (write the release announcement) or even for each sub-item? We didn't have that until now.

@pljones
Copy link
Collaborator

pljones commented Sep 12, 2022

No - but if documentation needs updating, there will be a pull request. I like an issue for each pull request, so the pull request isn't just "floating".

@hoffie
Copy link
Member Author

hoffie commented Sep 12, 2022

No - but if documentation needs updating, there will be a pull request. I like an issue for each pull request, so the pull request isn't just "floating".

As far as I can see, we won't need to update existing documentation on the website for this issue (Qt6 bugfix). I had added the label as a reminder for the release announcement and have now removed it as it is properly tracked.
Did you mean to comment on #2841?

@pljones
Copy link
Collaborator

pljones commented Sep 12, 2022

Ah right, if it was just for the release announcement, then so long as there's a reminder there, then "done".

hoffie added a commit to hoffie/jamulus that referenced this pull request Sep 14, 2022
hoffie added a commit to hoffie/jamulus that referenced this pull request Sep 14, 2022
hoffie added a commit to hoffie/jamulus that referenced this pull request Sep 18, 2022
ann0see pushed a commit to ann0see/jamulus that referenced this pull request Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Server: Command-line-specified country ID interpretation changed after switch to 3.9.0/Qt6
4 participants