Skip to content

GUI: Improve Connect Dialog help texts#2121

Merged
pljones merged 1 commit intojamulussoftware:masterfrom
pljones:feature/connect-dialog-help-improvements
Nov 20, 2021
Merged

GUI: Improve Connect Dialog help texts#2121
pljones merged 1 commit intojamulussoftware:masterfrom
pljones:feature/connect-dialog-help-improvements

Conversation

@pljones
Copy link
Copy Markdown
Collaborator

@pljones pljones commented Nov 13, 2021

Short description of changes

Improvements to the Connect dialog help

Context: Fixes an issue?

Fixes some misalignment between display names and help text, adds some help text and updates descriptions to cover "new" functionality.

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

Translations will need updating.

Status of this Pull Request

Ready to merge but please review for style (maybe "client connection" doesn't need to mention "client", etc).

What is missing until this pull request can be merged?

Ready to merge.

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

@pljones pljones requested review from gilgongo and softins November 13, 2021 12:34
@pljones pljones force-pushed the feature/connect-dialog-help-improvements branch from 15ca137 to adf6b27 Compare November 13, 2021 12:35
Comment thread src/connectdlg.cpp Outdated
Comment thread src/connectdlg.cpp Outdated
Comment thread src/connectdlg.cpp Outdated
Comment thread src/connectdlg.cpp Outdated
Copy link
Copy Markdown
Member

@gilgongo gilgongo left a comment

Choose a reason for hiding this comment

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

I think one day we could do with reviewing all this UI help. Some of it could benefit from being a bit more informative I think - eg "Use the Directory dropdown change directory" is rather ... thin :-)

Comment thread src/connectdlg.cpp Outdated
@pljones pljones force-pushed the feature/connect-dialog-help-improvements branch from adf6b27 to 789c864 Compare November 13, 2021 14:07
Comment thread src/connectdlg.cpp Outdated
@pljones pljones force-pushed the feature/connect-dialog-help-improvements branch from 789c864 to 86978ae Compare November 13, 2021 14:21
@pljones pljones requested a review from gilgongo November 13, 2021 14:28
@pljones pljones dismissed gilgongo’s stale review November 14, 2021 19:44

Changes applied

Comment thread src/connectdlg.cpp
QString().setNum ( DEFAULT_PORT_NUMBER ) +
tr ( ". The field will "
"also show a list of the most recently used server addresses." );
"address or URL using a colon as a separator, e.g, %1. "
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we include something about literal IPv6 addresses here? Because it contains colons, a literal IPv6 address must be enclosed in [ ], and any optional :port added after the ].

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should also add that into the docs too. Will give that a go. As to the text, what's the %1 variable here (I'm on a Mac and the help text isn't showing...)? If it's jamulus.example.com:22124 then maybe have jamulus.example.com:22124 (Note: IPv6 addresses must be enclosed in square brackets)

Copy link
Copy Markdown
Collaborator Author

@pljones pljones Nov 15, 2021

Choose a reason for hiding this comment

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

Personally, I'd rather keep it short and simple here, with the documentation for IPv6 providing the cases where something specific must be provided. It's already a bit long...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Putting the cursor into the Server Address field and pressing Shift-F1 on my Linux build gives
image
Right-click, choose What's This? on the "Server Address" label gives the same text (of course).

Shift-F1 on my Windows box:
image

Copy link
Copy Markdown
Member

@gilgongo gilgongo Nov 16, 2021

Choose a reason for hiding this comment

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

Hm. This is why I think all this is due for a re-jig now we have the docs in any case. It should really say something like, "Enter the URL or IP address of any private server you have access to." The help popups should just disambiguate the UI element that's described in the docs (and so that we don't spend efforts keeping the two in sync). But then that would apply to all of them, so that's not part of this ticket. I'd just leave it for now and I'll put the IPv6 thing in the docs.

@pljones
Copy link
Copy Markdown
Collaborator Author

pljones commented Nov 20, 2021

@softins @gilgongo Any other concerns?

Copy link
Copy Markdown
Member

@softins softins left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

Copy link
Copy Markdown
Member

@gilgongo gilgongo left a comment

Choose a reason for hiding this comment

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

Yes - I think we should do a general review of all of the UI help text at some point though.

@pljones pljones merged commit 5c65678 into jamulussoftware:master Nov 20, 2021
@pljones pljones deleted the feature/connect-dialog-help-improvements branch November 20, 2021 22:58
@pljones pljones added this to the Release 3.8.2 milestone Feb 13, 2022
hoffie added a commit to hoffie/jamulus that referenced this pull request Feb 13, 2022
@hoffie hoffie changed the title Improve Connect Dialog help texts GUI: Improve Connect Dialog help texts Feb 13, 2022
softins added a commit that referenced this pull request Feb 13, 2022
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