On connect dialog, in the directory list, remove 'Custom' and add all custom central servers #1894

jp8 · 2021-06-28T10:15:55Z

On the connect dialog, in the directory (custom central server) list,

remove Custom from list
add the user's custom central servers to the list

Addresses discussion #1869
Is an alternative to pull request #1884

It is a change to the UI. If the pull request is accepted, I will make a corresponding pull request to the website documentation.

Working implementation

It's a UI change so testing and feedback are appreciated. It is one alternative UI among several that are under discussion in #1869 and #1884

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

Last steps:

Read through code and test it (should be done already)
Add @jp8 to contributor page
Add Change log entry
Get two approvals

ann0see · 2021-06-28T10:43:42Z

Hi @jp8 thank you for your contribution. The style doesn't seem to be right. Could you please have a look at clang format?

Also would it be possible to have screenshots?

jp8 · 2021-06-28T10:51:37Z

Thanks @ann0see

clang format is in progress.

Screenshots copied from #1884

ann0see · 2021-06-28T10:57:44Z

Ok. Wasn't up to date with the other PR. Once the artifacts are built - and I find time - I'd be happy to test your proposal. From a first glance it looks interesting ;-)

ann0see · 2021-06-28T10:59:21Z

BTW, you can run clang-format -i /path/to/files locally to fix the code style.

jp8 · 2021-06-28T11:01:32Z

Thanks - I did try that approach several weeks ago but got errors, likely due to an old clang-format in Debian 10.

jp8 · 2021-06-28T11:13:52Z

I tried to write it in a way that would remain compatible with all the other proposals that are still under discussion in #1884

ann0see · 2021-06-28T11:21:20Z

Yeah. Not a big deal since the check now seems to be ok.

After having compiled it I'd say I'm quite happy with this change!
Sure, it doesn't follow the original request from the discussion, however, I think moving directory management from settings to the connect dialog might become a religious discussion ;-).

What about:

Reviewing and merging this PR
Adding an "edit" button which opens settings in the connect dialog?

ann0see

Quite small but effective ;-)

src/connectdlg.cpp

jp8 · 2021-06-28T12:07:14Z

2\. Adding an "edit" button which opens settings in the connect dialog?

I do think an edit button or link that opens the settings dialog to the correct page is the likely best way to get to the management interface.

But there might be a lot of discussion and ideas around that, and I don't think this small pull request depends on it. Could we save it as a separate topic ?

src/connectdlg.cpp

src/settings.h

src/connectdlg.cpp

src/settings.h

dcorson-ticino-com

Sorry I can't really comment, this week on vacation with only a cellphone.

src/settings.cpp

pljones · 2021-06-29T15:47:45Z

By the way, feel free to

git checkout master
git pull upstream master
git pull upstream master --tags --force
git push
git push --tags --force
git checkout -

to refresh your local git and then

git rebase -i master

and squash history (use "fix" if you want). I don't think any of us review on a commit by commit basis and the extra commit history will need cleaning up before it's merged.

src/connectdlg.cpp

src/settings.cpp

- remove Custom from list - add the user's custom central servers to the list Fix code formatting More fixes to coding style Fixed code comment Improved code comments to clarify how iCustomDirectoryIndex is used. Further code comments

rename cbxCentServAddrType to cbxDirectoryServer Fix code formatting renamed handler OnCentServAddrTypeChanged -> OnDirectoryServerChanged

…m central servers

ann0see

Please use pljones suggestion to rebase your work: #1894 (comment)

Doing it with a merge makes stuff complicated

gilgongo · 2021-06-30T11:27:20Z

@hoffie

I still agree with the arguments there.

Sorry to be a pain but in the interests of clarity, can you detail why you agree, against the points made here? #1884 (comment)

jp8 · 2021-06-30T11:34:14Z

Please use pljones suggestion to rebase your work: #1894 (comment)

Doing it with a merge makes stuff complicated

I am really sorry to be so new to git. I did follow the steps from pljones but on my next attempt to push, it left me with:

hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart.

...and asked me to pull from my feature branch to local and then merge conflicts. I am guessing that after following the steps, I should have pulled from my feature branch, before doing any further work.

ann0see · 2021-06-30T11:48:09Z

No. That’s what git push --force is used for ;-). However you might want to give https://git-scm.com/docs/git-rebase a short read.

I‘d suggest to undo the merge, and then force push everything.

jp8 · 2021-06-30T14:27:42Z

I‘d suggest to undo the merge, and then force push everything.

OK I will figure out how to do that. If I understand right, the goal is to take the 20 commits that are now on the feature branch in my fork, and condense them into just one or a few commits... is it right ?

ann0see · 2021-06-30T14:56:59Z

Kind of. Yes:
Goal:

Multiple (or one) commit/s based on the latest master code.

You‘d git checkout this commit: 79559e5

git push HEAD to your feature branch (https://stackoverflow.com/a/40929378) and then use git rebase -i to bring the commits together.

jp8 · 2021-07-02T12:13:41Z

git push HEAD to your feature branch (https://stackoverflow.com/a/40929378) and then use git rebase -i to bring the commits together.

I think I've done all this. I condensed it to 7 commits. Is it ok ?

ann0see

From a first glance this looks much better now ;-)

jp8 · 2021-07-05T09:11:45Z

Is there anything more I need to do, in order to get this merged ?

ann0see · 2021-07-05T10:23:34Z

There are still some unresolved review comments above.

centservaddrtype is set to AT_CUSTOM.

jp8 · 2021-07-05T11:50:03Z

ok all done i think.

ann0see · 2021-07-05T12:09:00Z

Last steps:

Read through code and test it (should be done already)
Add @jp8 to contributor page
Add Change log entry
Get two approvals

Anything else? Could you please copy and paste this check list in your first post above?

jp8 · 2021-07-05T13:39:46Z

* [ ]  Add @jp8 to contributor page

I was already on this list, from contributions I gave directly to Volker in the past.

pljones

I'm happy with it now. (Subject to the TODO item getting done ASAP.)

ann0see

Looks OK to me. I‘ve tested an early version and the code looks ok.

ann0see · 2021-07-05T18:56:19Z

This should be squash&merged.

pljones · 2021-07-05T21:42:24Z

I couldn't get the github UI to do what I wanted, so I did it locally, hence I'm going to close this.
62a9c38

ann0see · 2021-07-06T06:34:36Z

There’s an arrow next to the merge button with which you can change it to squash and merge?

pljones · 2021-07-06T07:39:41Z

There’s an arrow next to the merge button with which you can change it to squash and merge?

I don't think that lets you adjust the commit message and it's as if the branch never existed and a single commit to master happened directly. At least I think that's what happened when I first tried it. I wanted to show a merge from a branch with a single commit from jp8 with a sane commit message.

jp8 · 2021-07-06T08:16:23Z

So at the end I should have sqashed again, into a single commit ? (just so I know for next time...)

pljones · 2021-07-06T21:04:11Z

Yeah, it can sometimes help to keep the history (it recently let me split a change into to two PRs) but generally, once final, one commit is preferred.

ann0see requested a review from dcorson-ticino-com June 28, 2021 11:21

ann0see reviewed Jun 28, 2021

View reviewed changes

src/connectdlg.cpp Outdated Show resolved Hide resolved

src/connectdlg.cpp Show resolved Hide resolved

src/connectdlg.cpp Outdated Show resolved Hide resolved