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

#3233 UI: Common method to delete server and custom directory entries #3260

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

AdamGLIN
Copy link
Contributor

@AdamGLIN AdamGLIN commented Apr 3, 2024

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.

Short description of changes

CHANGELOG: Client: Add "Delete Entry" button to Advanced Settings, Custom Directories

Context: Fixes an issue?

#3233

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

Status of this Pull Request

What is missing until this pull request can be merged?

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 ann0see changed the title first commit to resolve the issue #3233 WIP: resolve #3233 Apr 3, 2024
@ann0see
Copy link
Member

ann0see commented Apr 3, 2024

Thanks for opening this PR. It's a draft/WIP so it needs discussion. @pljones Should be most familiar with this.

@ann0see
Copy link
Member

ann0see commented Apr 3, 2024

Could you please also post your questions here? Best right in the code tab close to where the question arises.

@AdamGLIN
Copy link
Contributor Author

AdamGLIN commented Apr 3, 2024

Hello,

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 !

@softins
Copy link
Member

softins commented Apr 3, 2024

Thanks Adam! I started to look at your commit and questions earlier today, but got diverted onto other things. I will have a look soon, hopefully tomorrow!

src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved
@pljones
Copy link
Collaborator

pljones commented Apr 16, 2024

Looks good to me, now. I've kicked off the build.

Thanks for all the time you've spent on this! :)

@AdamGLIN
Copy link
Contributor Author

Thank you for your patience and your advices :)

@pljones
Copy link
Collaborator

pljones commented Apr 17, 2024

Just tried the Windows version from the build artifacts and it looks okay to me.

@pljones pljones added this to the Release 3.11.0 milestone Apr 17, 2024
@pljones pljones linked an issue Apr 17, 2024 that may be closed by this pull request
@ann0see ann0see self-requested a review April 17, 2024 21:15
@ann0see
Copy link
Member

ann0see commented Apr 17, 2024

This needs some squashing and rebasing, I believe. I'll look into it once I have time.

@pljones pljones changed the title WIP: resolve #3233 #3233 UI: Common method to delete server and custom directory entries May 15, 2024
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.

I've tried this on my Pi and my Mac. Both ok, so happy to approve.

There don't seem to be any merge conflicts, so a squash merge could be done without needing to rebase. Or else I'd be happy to rebase and squash manually first.

@softins
Copy link
Member

softins commented Jun 11, 2024

OK, I'll rebase it onto main, which now has the Mac legacy Qt update, let it build, and then check it on my Mac.

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.

I've just loaded the Mac legacy version built with Qt 5.15 and noticed a couple of issues which need to be addressed first. I'm happy to look at these.

  1. The button is too narrow for the delete character (it's ok on other platforms and was ok with Qt 5.9). This is the same as I reported in "Delete server" button doesn't show properly on macOS #3241
image
  1. If I type in a custom directory address and finish with "Enter", the field goes blank and the address isn't added. I need to use Tab, or to click outside the field, to make it lose focus. I did see some comments about pressing Enter, but thought they had been resolved. Evidently not, at least on some platforms.

@pljones
Copy link
Collaborator

pljones commented Jun 12, 2024

Yep Qt on MacOS certainly seems to come with its own UI layout peculiarities that aren't present on other platforms.

The action/focus trigger for updating the list has also been a bit hard to pin down: it works right sometimes on both Windows and Linux for me. It also works wrong sometimes, too... Or has done (it's a while since I ran Qt5 - it might be fixed in Linux/Windows Qt6).

(I'm thinking of getting the Android to Qt6 work done - to see if it helps with the other Android issues we have open. I tried it over the weekend and it mostly looks like permissions stuff... but that doesn't look trivial to fix.)

@softins
Copy link
Member

softins commented Jun 12, 2024

I fixed the issue with Enter. The problem was that pressing Enter within the combi box was causing a button push to delete, because the default setting for autoDefault appears to be true. Having set autoDefault explicitly to false for the delete button in clientsettingsdlgbase.ui, the Enter key works correctly for just finishing editing.

Will investigate the button appearance on Mac later.

@softins
Copy link
Member

softins commented Jun 13, 2024

For the delete buttons, I needed to change the horizontal size policy from "Maximum" to "Fixed", so that the buttons were definitely wide enough, but not allowed to grow. Need to check they are ok on all platforms.

@softins
Copy link
Member

softins commented Jun 13, 2024

I've tried the latest build above on Mac Legacy, Windows and Pi Linux. Not current Mac though.

The button width is now fine on Mac Legacy, but it's a bit wider than ideal on Windows and Linux. Must be scaling differences. It may be acceptable, but if not, we would need to add code to adjust the width of those buttons at runtime for Mac only. Thoughts?

@softins softins requested review from pljones and softins June 13, 2024 13:23
@pljones
Copy link
Collaborator

pljones commented Jun 13, 2024

Maybe set the size policy to maximum only for Mac if that's where it's needed? (Quite why Qt can't get their cross-platform UI toolkit to work for something you'd have though to be pretty obvious like that, I have no idea. Qt's meant to hide those kinds of differences...)

@softins
Copy link
Member

softins commented Jun 13, 2024

Maybe set the size policy to maximum only for Mac if that's where it's needed?

It's the other way around. It was previously maximum only, but that allows the button to be shrunk as much as needed. But on Mac from 5.12 on, the button was shrinking right down. My guess is that it couldn't correctly calculate the size of the contained text when it was just a Unicode character for backspace, so took the text width as 0. I found I needed to specify Fixed so that both maximum and minimum were constrained.

Either way, to fix it we would need to add conditional runtime code for Mac only, as there is no way to put architecture conditionals in a .ui file. The question is, is it worth doing?

I have wondered whether it might be worth putting an image in the button instead of a Unicode character? e.g. a wastebasket or trashcan?

@softins
Copy link
Member

softins commented Jun 13, 2024

A further thought: maybe merge this PR without the button width fix, since that applies to both the settings and connect dialogs? The connect dialog is arguably outside the scope of this PR. Then raise a separate PR to fix #3241 in both places for Mac?

@pljones
Copy link
Collaborator

pljones commented Jun 14, 2024

OK by me.

So, dropping the third commit...

AdamGLIN and others added 2 commits June 14, 2024 16:26
In Advanced Settings, changes the method of deleting a custom directory
to use a delete button, for consistency with the similar method in Connect.
@pljones pljones force-pushed the customDirectoriesButton branch 2 times, most recently from 84c6eb6 to 5ec1b26 Compare June 14, 2024 15:28
@pljones pljones merged commit f965518 into jamulussoftware:main Jun 14, 2024
9 checks passed
@ann0see ann0see removed their assignment Jun 15, 2024
@ann0see ann0see added the needs documentation PRs requiring documentation changes or additions label Jun 20, 2024
@ann0see
Copy link
Member

ann0see commented Jun 20, 2024

Ok. So this also needs new screenshots for the docs potentially.

@pljones
Copy link
Collaborator

pljones commented Jun 21, 2024

Linux:
image

Windows:
image

Translations, too? There's new "What's This?" text, IIRC.

@ann0see
Copy link
Member

ann0see commented Jun 21, 2024

Yes. But that should show up automatically

@pljones pljones removed the needs documentation PRs requiring documentation changes or additions label Aug 4, 2024
@pljones
Copy link
Collaborator

pljones commented Aug 4, 2024

jamulussoftware/jamuluswebsite#1002 raised for documentation.

@ann0see ann0see removed their request for review August 4, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

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