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.

Sign up for GitHub

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

Jump to bottom

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

Open
pljones opened this issue Feb 12, 2024 · 5 comments · May be fixed by #3260
Open

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

pljones opened this issue Feb 12, 2024 · 5 comments · May be fixed by #3260
Assignees
@AdamGLIN
Labels
feature request Feature request
Milestone
Release 3.11.0

Comments

@pljones
Copy link
Collaborator

pljones commented Feb 12, 2024

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.
@pljones pljones added the feature request Feature request label Feb 12, 2024
@pljones pljones added this to the Release 3.11.0 milestone Feb 12, 2024
@ann0see
Copy link
Member

ann0see commented Feb 12, 2024

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

@AdamGLIN
Copy link

Hello !

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

Thanks in advance

@softins
Copy link
Member

softins commented Mar 27, 2024 •
edited

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 added a commit to AdamGLIN/jamulus that referenced this issue Apr 3, 2024
@AdamGLIN
first commit to resolve the issue jamulussoftware#3233
2780a48 
I added a horizontal box layout which include the box and the delete button. I wrote a fonction to implement the delete button.

It works well when we change the focus with the mouse to add a new directory but I can't make the enter key work : it delete the input and nothing happend.
I tried to add the QLineEdit::returnPressed signal but it changed nothing.
I struggle to understand the former use of the QComboBox::activated signal since I commented it and nothing seems to have changed.

TODO :
Allowing to add directory via enter key.
Improve spacers in the ui.
@AdamGLIN
Copy link

AdamGLIN commented Apr 3, 2024

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
Copy link
Member

ann0see commented Apr 3, 2024 •
edited

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.

AdamGLIN added a commit to AdamGLIN/jamulus that referenced this issue Apr 5, 2024
@AdamGLIN
second commit to resolve the issue jamulussoftware#3233
37d4c74 
I followed the suggestions made py @pljones and I changed a size hint of a spacer in the UI to make sure that everything is evenly spaced.

TODO :

Allowing adding directory via enter key
AdamGLIN added a commit to AdamGLIN/jamulus that referenced this issue Apr 7, 2024
@AdamGLIN
third commit to resolve jamulussoftware#3233
08f40dd 
I used flags to allow the use of only one function for the delete button and the list itself, I deleted a ":" in
the label (other labels didn't use ":") and I added a tabstop for the button
AdamGLIN added a commit to AdamGLIN/jamulus that referenced this issue Apr 16, 2024
@AdamGLIN
forth commit to resolve jamulussoftware#3233
edcd230 
I added return in case the data isn't valid. I also added a ":" to avoid translations problems.
@pljones pljones linked a pull request Apr 17, 2024 that will close this issue
#3233 UI: Common method to delete server and custom directory entries #3260
Open
5 tasks
@pljones pljones linked a pull request Apr 17, 2024 that will close this issue
Open
5 tasks
@pljones pljones modified the milestones: Release 3.11.0, Release 3.12.0 May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AdamGLIN AdamGLIN

Labels
feature request Feature request
Projects
Tracking
Status: In Progress
Milestone
Release 3.11.0
Development

Successfully merging a pull request may close this issue.

#3233 UI: Common method to delete server and custom directory entries AdamGLIN/jamulus
4 participants
@pljones @softins @ann0see @AdamGLIN