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

Use the default server name in the mixer #2173

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Dec 15, 2021

Short description of changes

Edge-case for user using no server details and clicking "connect" from the connect window. They could be looking at any of the directories when they hit connect. For example:
image

Hitting "Connect" used to title the mixer "Directory Server" (or translation). Now it titles it with the default directory name as more of a clue:
image

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

No.

Translations needed.

Status of this Pull Request

Working as expected.

What is missing until this pull request can be merged?

Should just work (except for translations changing).

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

pljones commented Dec 15, 2021

Awaiting this over on https://github.com/pljones/jamulus

  • patch/ocd-reorder-of-settings
  • patch/ocd-reorder-of-serverdlg-bits
    These two pretty much don't change anything, just put the code through a "mental sort", so I could follow it better. Mostly because they're areas I'm planning to change... and indeed...
  • feature/server-list-persistence-ui
    Adds a UI to allow the server list persistence file to be set. Plus changing rather a lot of other stuff to make changes in the UI not need the server stopped and started.

@ann0see
Copy link
Member

ann0see commented Dec 16, 2021

How/where is AT_DEFAULT set?

Also: shouldn’t we rename the directory server at any genre to match your label?

@pljones
Copy link
Collaborator Author

pljones commented Dec 16, 2021

How/where is AT_DEFAULT set?

grep is your friend. src/util.h line 547.

Also: shouldn’t we rename the directory server at any genre to match your label?

Not given this is for an edge case, I don't think. I'm only changing it from what it was so it's clearer you're not necessarily somewhere related to what you were looking at.

@ann0see
Copy link
Member

ann0see commented Dec 16, 2021

Hmm. I think the main problem is that connecting to the Any Genre 1 directory while being in Any Genre 2 or similar is the confusing part. And this can't be fixed with this (rather visual) approach. OFC, it's much better with your approach, but I think we should rather change functionality of this button if we're in another genre...

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.

Although untested, the code looks simple enough to approve.

@ann0see
Copy link
Member

ann0see commented Dec 16, 2021

BTW: Thanks for the "next big thing" work on Jamulus.

@pljones pljones requested a review from softins December 16, 2021 21:33
Copy link
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.

Just tried this out and it works fine, so I'm happy to approve, as it doesn't change existing functionality.

But I do think the existing functionality is wrong. An improvement would be to connect to the directory server of the currently-displayed list, but I think the right behaviour going forward would be: if no server in the list is selected, AND the server address box is blank, the Connect button is disabled.

@pljones
Copy link
Collaborator Author

pljones commented Dec 17, 2021

Just tried this out and it works fine, so I'm happy to approve, as it doesn't change existing functionality.

But I do think the existing functionality is wrong. An improvement would be to connect to the directory server of the currently-displayed list, but I think the right behaviour going forward would be: if no server in the list is selected, AND the server address box is blank, the Connect button is disabled.

Yes, it's a bit weird now, the way it is. (Before multiple servers, it just connected to Volker's server, of course, which sort of made sense.)

I'd like to have "Escape" work on the connect dialog along with getting rid of this "default" behavour, too. (There was a patch to turn off "Escape" on all the dialogs - I'm not entirely sure why...)

@pljones pljones merged commit 78a5b19 into jamulussoftware:master Dec 17, 2021
@pljones pljones deleted the bugfix/use-default-server-name-in-mixer branch December 17, 2021 17:10
@gilgongo gilgongo added this to Triage in Tracking (old) via automation Jan 17, 2022
@gilgongo gilgongo moved this from Triage to Done in Tracking (old) Jan 17, 2022
@gilgongo gilgongo added this to the 3.8.2 milestone Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants