Ability to delete Server and Directory from a list should be the same

[pljones]

What is the current behaviour and why should it be changed?

In the Client:

• In the Settings Dialog, Advanced tab, the Custom Directories list allows adding and removing of Directories. To remove a Directory from the list, the edit box must be cleared and enter pressed or focus moved to another field with the box empty.
• In the Connect Dialog, the Server list (near the "Connect" button) allows adding and, recently, removing of Servers. To remove a Server, from the list, the <[X] button is pressed.

In the Server, on the Options tab:

• The recording directory can be set or cleared.
• The server list filename (when running as a Directory) can be set or cleared.

In both cases, to clear the value, the <[X] button is pressed.

The Settings Dialog, Advanced tab, Custom Directories list isn't as easy to use or obvious in how to remove an entry as the others.

Describe possible approaches

The fields in the Server are read-only, with a dialog to select a new value for the field. The dialog itself cannot be used to clear the value. So having the extra "clear" button is necessary.

The Client Connect dialog has used the same approach for the Server list.

The options are:

• Add a <[X] button to the Advanced, Custom Directories list to allow deleting the selected value the same was as the Connect dialog and remove the existing method.
• Add a <[X] button to the Advanced, Custom Directories list to allow deleting the selected value the same was as the Connect dialog, whilst retaining the existing method.
• Add the same "clear the field and move focus" method to the Connect dialog (hitting enter won't work here as it would attempt to connect, thus connecting to the current Genre Directory as no server was selected)

I don't think the Server buttons should be changed.

I think the best method is to switch the Advanced, Custom Directories list to use the <[X] button.

Has this feature been discussed and generally agreed?

See #3159 (comment) for earlier discussion.

[ann0see]

@gilgongo @mcfnord what do you think? (Please stay on topic)

[AdamGLIN]

Hello !

I planned on implementing the first option given by @pljones. Is there anybody assigned yet ?

Thanks in advance

[softins]

Hi @AdamGLIN, thanks for your comment and offer!

I planned on implementing the first option given by @pljones. Is there anybody assigned yet ?

I agree that the first option is the one to go for.

I'm not aware of anyone having been assigned, and I can't see any branch in @pljones' fork that relates to this.

So I'd say go for it, submit the PR, and we'll review it. Thanks again!

[AdamGLIN]

Hello,

I just pushed to my branch.

It works for the main part, I mostly followed this commit.

However, I have some issues :

• I can't figure out why the QLineEdit::editingFinished signal that we use to notify the changes, don't seems to work when I press the enter key (I read that it was supposed to). (src/clientsettingsdlg.cpp, line ~700)
• I don't understand the use of the QComboBox::activated signal since I commented it and it changed nothing. Also I read that the signal was sent when the user clicks on an item of the list and I think that it was not a interesting behavior for this feature because the order doesn't matter. Tell me if I'm wrong. (src/clientsettingsdlg, line ~700)
• I found out that we use the StringFiFoWithCompare method (src/util.h, line ~170) to update the list without repetition (I think) for both directories and server address lists (src/clientsettingsdlg, line ~1060). However, there is a loop introduced in this commit (src/connectdlg.cpp, line ~750) to avoid repetition, and hence it is useless. For the first change I decided just to copy the loop and not think to much about it. What do you think about it ? Did I miss something ?

I have still things to improve such as the interface (boxes are not equally spaced), but I would like your opinion to continue this way !

Thanks for your time !

[ann0see]

Thanks for working on it. I believe it's best you open a PR to allow easier review.

It's always possible that there's a bug/inefficiency in the current code on master.