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

Display server versions in Connect dialog #3416

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

softins
Copy link
Member

@softins softins commented Oct 25, 2024

Short description of changes

Adds a Version column to the server list in the Connect dialog, and populates this once for each server in the list.

CHANGELOG: Client: Display version for each server in the Connect dialog

Context: Fixes an issue?

This is a proof of concept in response to the discussion at https://github.com/orgs/jamulussoftware/discussions/3382#discussioncomment-10941423, although not itself related to access control.

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

On the website, any screenshots of the Connect dialog should be updated.

Status of this Pull Request

Ready for review. We can discuss whether to put it in 3.12.0 (if there is one) or 4.0.0

What is missing until this pull request can be merged?

Only a decision about when. We should also check the column width is ok on all platforms.

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
Copy link
Member Author

softins commented Oct 25, 2024

Example screen shot:

image

@softins
Copy link
Member Author

softins commented Oct 25, 2024

If we add this, I think we should also consider again adding #1940

@softins softins force-pushed the display-server-versions branch 2 times, most recently from faba634 to 0193705 Compare October 25, 2024 17:11
@ann0see
Copy link
Member

ann0see commented Oct 25, 2024

I'd like it to be a bit more out of the way (last column) or entirely only showing up on hover (preferred as it's not essential)

@softins
Copy link
Member Author

softins commented Oct 25, 2024

Well we could rearrange the columns (should probably create some defines or an enum for column numbers, so they can be changed only in one place in the code, matching the .ui), but I don't think hover is a good idea, especially if we want to target tablets and phones too, which don't have hover.

@softins
Copy link
Member Author

softins commented Oct 25, 2024

Personally, I prefer it where I showed above.

@ann0see
Copy link
Member

ann0see commented Oct 25, 2024

It's not essential. Ping is more important for example. So it should not be second in my opinion

@softins
Copy link
Member Author

softins commented Oct 26, 2024

Updated view. At least with an enum for column values it is easier to try different ways.

image

Updated column indexes to match. Version column not yet populated.
This helps the first ping be a bit more accurate, as firewall/NAT
sessions will already have been opened by the version request.
@ann0see
Copy link
Member

ann0see commented Nov 3, 2024

The linux standard window size seems to hide the version in the connect dialog. Not really a problem, but just so you know.

LVC_VERSION, // server version
LVC_PING_MIN_HIDDEN, // minimum ping time (invisible)
LVC_CLIENTS_MAX_HIDDEN, // maximum number of clients (invisible)
LVC_COLUMNS // total number of columns
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively we could most likely also calculate this programmatically. It could go out of sync in future due to a mistake?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a better approach would be to remove the headings from the widget in the .ui file and to set them in the code in the same section as we set the widths. I did consider this, but didn't get around to looking up the correct way to do so. Then the enum would be the one source of truth for column order.

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 and despite some small comments, it looks ok.

@softins
Copy link
Member Author

softins commented Nov 3, 2024

The linux standard window size seems to hide the version in the connect dialog. Not really a problem, but just so you know.

That's interesting, I didn't notice that issue under Linux on my Pi; the connect window was wide enough to show the version column. Is the connect window size remembered in the .ini file?

@ann0see
Copy link
Member

ann0see commented Nov 4, 2024

Yes. The ini file has the correct size. I deleted the ini file to test the size and that's too small.

@softins softins marked this pull request as draft November 4, 2024 11:28
@softins softins self-assigned this Nov 4, 2024
@softins
Copy link
Member Author

softins commented Nov 4, 2024

The linux standard window size seems to hide the version in the connect dialog. Not really a problem, but just so you know.

I've updated the default width in connectdlgbase.ui. Once there is an ini file it will just get overwritten by the saved value.

@softins softins marked this pull request as ready for review November 4, 2024 18:05
@mcfnord
Copy link
Contributor

mcfnord commented Nov 13, 2024

Maybe we could prepare a documentation page that's just a table that lists the advanced features someone might enjoy starting with each particular server version. Presumably this would involve condensing release notes for each version into a single at-a-glance reference.

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.

3 participants