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

Reorder of serverdlg bits #2177

Merged

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Dec 22, 2021

Short description of changes

This starts to get to making actual changes in behaviour of the server dialog code.

I STRONGLY recommend viewing each commit separately before viewing the full diff. The aim was to keep each change small, self-contained and then test so that I was confident I'd not broken anything along the way.

(Admittedly, skimming the full diff now, it doesn't look too bad...)

Context: Fixes an issue?

The aim is multifold but primarily to make it easier to tie the UI as displayed to the code that implements it. This means code gets moved around quite a lot, meaning the diff looks like inserts/deletes, unfortunately -- hence the recommendation above.

It also fixes a few bits (like adding missing "What's This" help).
image -> image

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

New / changed translation(s). May need new screenshots. (I've been using it too long to remember, now.)

Status of this Pull Request

Ready to merge (IMO).

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 22, 2021

And once this is done

@pljones
Copy link
Collaborator Author

pljones commented Dec 22, 2021

image
Something I've not done is change "Custom Directory" to (or back to...) "Custom Directory Server Address"... but in context here, I think that would be a good idea.

(OK, done as part of feature/server-list-persistence-ui, along with slight adjustment to the "Language" dropdown, which looked a bit odd before.)

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 a few initial comments for consideration before doing a final review. Looks good in general.

src/serverdlg.cpp Show resolved Hide resolved
src/serverdlg.cpp Show resolved Hide resolved
src/serverdlg.cpp Show resolved Hide resolved
@ann0see
Copy link
Member

ann0see commented Dec 22, 2021

Hmm. Not 100% sure about the concept of setting a list to "Unregistered". Basically it seems a bit unintuitive if we think about the private/registered(=public) server concept. How do we ensure that the user knows "Unregistered" = private server? I think the behaviour we had until now was a little bit more intuitive.

@ann0see
Copy link
Member

ann0see commented Dec 22, 2021

… and I mean, putting the custom server textbox into another tab feels a bit strange.

@pljones
Copy link
Collaborator Author

pljones commented Dec 23, 2021

Hmm. Not 100% sure about the concept of setting a list to "Unregistered". Basically it seems a bit unintuitive if we think about the private/registered(=public) server concept. How do we ensure that the user knows "Unregistered" = private server? I think the behaviour we had until now was a little bit more intuitive.

It solves an existing defect - go read the discussions.

(It's probably easier to wait until this get to PR so you can try running it -- or jump on the latest build on my branch.)

@pljones
Copy link
Collaborator Author

pljones commented Dec 23, 2021

… and I mean, putting the custom server textbox into another tab feels a bit strange.

It hasn't moved.

@ann0see ann0see merged commit e1e6d8e into jamulussoftware:master Dec 28, 2021
@pljones pljones deleted the patch/ocd-reorder-of-serverdlg-bits branch December 28, 2021 22:03
@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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants