Fixes #1421: Add "delete server" button to connect dialog

[pljones]

Short description of changes

Adds a "delete server" tool button to the connect dialog, in the same style as the server dialog, to delete the currently selected server. There's no "undo".

Selected server gets deleted. Next server in list is then current in field. On exit, the updated list is saved.

CHANGELOG: Add "delete server" button to connect dialog

Context: Fixes an issue?

Fixes #1421

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

Yes - screenshots and translation strings need updating.

Status of this Pull Request

Tested locally and appears to work as expected.

What is missing until this pull request can be merged?

Code and UI review.

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

[ann0see]

@gilgongo Could you please comment on the UX here? How can we avoid confusion with the connect button?

[softins]

@gilgongo Could you please comment on the UX here? How can we avoid confusion with the connect button?

Personally, I would prefer something like a red X instead of a black backspace symbol.

[pljones]

Personally, I would prefer something like a red X instead of a black backspace symbol.

Should we change the Server UI, too? (I didn't find a button with a red X on any more meaningful (I think "stop" or "wrong" rather than "delete") - though I admit the symbol chosen isn't particularly obvious.)

[ann0see]

If it's changed here, it should be changed everywhere

[softins]

Personally, I would prefer something like a red X instead of a black backspace symbol.

Should we change the Server UI, too? (I didn't find a button with a red X on any more meaningful (I think "stop" or "wrong" rather than "delete") - though I admit the symbol chosen isn't particularly obvious.)

Ah, I hadn't remembered it is already used in the Server UI (I so seldom use a server with a GUI!)

[pljones]

Ah, I hadn't remembered it is already used in the Server UI (I so seldom use a server with a GUI!)

Yeah, it means "clear field" there (where the user can't directly type into the field), so there is a distinction.

[ann0see]

✗ https://www.compart.com/en/unicode/U+2717

⊗

https://unicodeplus.com/U+2297

Could be options

Not preferred but possible:

https://www.compart.com/en/unicode/U+1F5D1

https://stackoverflow.com/questions/5353461/unicode-character-for-x-cancel-close

[pljones]

As it says on the last reference, "X" tends to imply cancel - rather than remove or delete. So I'm not keen on anything looking just like an "X". The character I've used is called "erase to the left", which is - essentially - what it does. https://www.fileformat.info/info/unicode/char/232b/index.htm

[ann0see]

Agree. You just don't see that often.

[ann0see]

Just a behavioral discussion point: If the entry is emptied with backspace, and then the new button is clicked, we jump to another entry from the list. I wouldn't expect that. Maybe the button should be grayed out then? Or just do nothing?

[ann0see]

(Also if we add this, the UX should be the same for custom directories, I'd say.)

[pljones]

(Also if we add this, the UX should be the same for custom directories, I'd say.)

That would want a separate issue raised... though I agree. The existing solution is inelegant.

[pljones]

Just a behavioral discussion point: If the entry is emptied with backspace, and then the new button is clicked, we jump to another entry from the list. I wouldn't expect that. Maybe the button should be grayed out then? Or just do nothing?

"Do nothing" is possible -- currently the redisplay happens anyway (as if the connect window had been closed then opened again).

I'd rather not have to catch every update to the text in the field and decide whether or not to disable the button.

[ann0see]

Ok. Then I think the do nothing should be implemented.

[softins]

Seems to work fine now.

[pljones]

Ok. Then I think the do nothing should be implemented.

Done.

[pljones]

Here's a comparison with the "Clear text" button in Calibre:

which, I think, is close enough to the glyph used here (inverse colours, bold text, maybe - and overlayed in the input box, which could be fun to replicate in Qt). (The button disappears when the field is empty.)

[pljones]

Oh, and apparently Qt Designer uses the same glyph. I wonder where it gets it...

[pljones]

Reverting to 10c5931, pretty much, but with width 24, bold and using the unicode glyph directly.

Linux local builds: QMake version 3.1, Qt version 6.6.0 and Qt version 5.15.2
and
Linux github artifact: Qt version 5.9.5


Server address drop down and new button same height. The text colour doesn't match the drop down arrow colour, unfortunately -- and, from what I can see in the manual, you can't refer to the widget colours, so I'd have to hard code it, which would break Qt theming.

Windows github artifact:
(My Windows local builds for Qt 6.6.0 crash out.)



Not perfect but close.

---

Given that Qt seems unable to provide a consistent, cross-platform experience for this, we're going to have to compromise.

[ann0see]

BTW: can you look at your git configuration? I believe something is wrong with your GPG key.

[pljones]

BTW: can you look at your git configuration? I believe something is wrong with your GPG key.

I've been trying to fix it... I know why it broke, just too long since I first got it working to remember what needs doing.

[ann0see]

I believe it's in GitHubs' documentation about GPG keys.

[pljones]

I believe it's in GitHubs' documentation about GPG keys.

Not the bit I needed... I don't know if I've fixed it until I create a new commit, though...

[softins]

@pljones what's the current status of this? I've just built it on my Pi and it seems to work fine.

I noticed that the branch pljones/feature/add-tooltips-to-connectdlg contains additions to this PR that haven't been pushed to it yet. Is it planned to add them before merging?

I also happened to notice that in Settings / Advanced Setup there is a combi-box with drop-down for multiple Custom Directories that could usefully have a similar button. I currently have two entries in that list, with no way I can see to remove one of them.

EDIT: I've just discovered that in the Custom Directories drop-down, I can backspace over the entry I don't want, and then click on the drop-down arrow, and the entry does get removed from the list. However, the same trick didn't work in the Connect dialog.

[ann0see]

I believe it's ok except for the button. But it's not too important.

[pljones]

EDIT: I've just discovered that in the Custom Directories drop-down, I can backspace over the entry I don't want, and then click on the drop-down arrow, and the entry does get removed from the list. However, the same trick didn't work in the Connect dialog.

Yes, this was the requested method for the Custom Directories drop-down, with a separate, different requested method for the Connect dialog. I think it would better if they both worked the same, but this request is only about the Connect dialog and, if the Custom Directories drop-down needs changing, that needs a new issues/PR raised.

[ann0see]

Probably going to approve it like this. I'm a bit unsure about the UX though the backspace icon is used in the server too, but maybe there's something clearer.

[ann0see]

However, the same trick didn't work in the Connect dialog.

@softins @pljones could you please open an issue on that?

[pljones]

@softins @pljones could you please open an issue on that?

Done.