Skip to content

Conversation

@softins
Copy link
Member

@softins softins commented Nov 6, 2025

Short description of changes

Corrects the sorting of the version column in the connect dialog when using --showallservers.
It does this by mapping the version number to a sortable text field and storing that in the data field of the widget item.
It also subclasses QTreeWidgetItem as CMappedTreeWidgetItem in order to provide an override for operator<
to sort on the data field instead of the text field.

Also increases the default column width for the version column.

CHANGELOG: Client: Fix sorting of version number in connect dialog with --showallservers

Context: Fixes an issue?

Fixes: #3542

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

No, it's just a bugfix.

Status of this Pull Request

Tested on RPi Linux. Ready for testing on other platforms.

What is missing until this pull request can be merged?

Review.

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

@softins softins added this to the Release 3.12.0 milestone Nov 6, 2025
@pljones pljones added this to Tracking Nov 6, 2025
@github-project-automation github-project-automation bot moved this to Triage in Tracking Nov 6, 2025
@pljones pljones moved this from Triage to In Progress in Tracking Nov 6, 2025
@pljones
Copy link
Collaborator

pljones commented Nov 6, 2025

image The column could still do with being wider. It's 18 characters maximum. I make that nearly as big as the server name.

@softins
Copy link
Member Author

softins commented Nov 6, 2025

The column could still do with being wider. It's 18 characters maximum. I make that nearly as big as the server name.

It's hard to be exact, as it varies between platforms. The main part of the version, and the alpha/beta/rc will be visible. Someone who is particularly interested in the dev commit numbers can always widen the field manually. It's no big deal to me either way.

@pljones
Copy link
Collaborator

pljones commented Nov 6, 2025

image

Otherwise, it looks pretty good. Note that André still brews something special and hacks the version number, which hits the -?(.*)$ regexp and goes into suffix - so it gets handled the same way as all the dev stuff.

My only proviso remains that the "alpha", "beta", "rc" order isn't fixed - we can interleave in any order and have "dev" servers out there mixed in between, of course. The way it looks now is that 3.12.0beta3 is "newer" than latest master, which it isn't. (That's why I wanted to change the Jamulus.pro file to capture the git commit time as well.)

@softins
Copy link
Member Author

softins commented Nov 6, 2025

My only proviso remains that the "alpha", "beta", "rc" order isn't fixed - we can interleave in any order and have "dev" servers out there mixed in between, of course. The way it looks now is that 3.12.0beta3 is "newer" than latest master, which it isn't. (That's why I wanted to change the Jamulus.pro file to capture the git commit time as well.)

The suffixes alpha, beta and rc sort before the bare version number (due to the <, =, > flag after the patch number), other suffixes sort after. I'm using the same technique as I did for Jamulus Explorer. For the order of alpha, beta and rc, it just so happens that alphabetical order corresponds with the order they happen. Just so long as we don't go beyond alpha9, beta9 or rc9, which is unlikely!

@pljones
Copy link
Collaborator

pljones commented Nov 6, 2025

Like I say, we could have alpha1, beta1, rc1 and then had to go back and do beta2 and beta3 before rc2. Not a big deal, though. (Switching in the git commit timestamp is now pretty easy, with the work here.)

@softins softins requested a review from ann0see November 7, 2025 00:01
@pljones pljones added the bug Something isn't working label Nov 7, 2025
{
x = "="; // bare version number
}
else if ( suffix.startsWith ( "rc" ) || suffix.startsWith ( "beta" ) || suffix.startsWith ( "alpha" ) )
Copy link
Member

Choose a reason for hiding this comment

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

  • Document somewhere on the release docs that the application actually filters based on these suffixes

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not so much a filter. All suffixes are shown. It's just that alpha, beta and rc sort before the base version, and all other suffixes sort after. I agree it's worth documenting somewhere suitable.

Copy link
Collaborator

@pljones pljones Nov 10, 2025

Choose a reason for hiding this comment

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

As a reminder in one of COMPILING.md or CONTRIBUTING.md -- or a comment at the top of Jamulus.pro perhaps.

@github-project-automation github-project-automation bot moved this from In Progress to Waiting on Team in Tracking Nov 9, 2025
@softins softins merged commit 6c89081 into jamulussoftware:main Nov 9, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from Waiting on Team to Done in Tracking Nov 9, 2025
@softins softins deleted the version-sort-new branch November 11, 2025 16:02
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

Status: Done

Development

Successfully merging this pull request may close these issues.

Connect Dialog with "--showallservers" does not sort "Version" normally

3 participants