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

Bugfix/#2723 self label checkboxes #2996

Merged

Conversation

pljones
Copy link
Collaborator

@pljones pljones commented Jan 21, 2023

Short description of changes

UI/UX issue #2723 requests that all check boxes be made "self-labelling". This makes the change to the relevant forms.

(It also causes the Server UI to start on "Server Setup" rather than "Options" consistently.)

CHANGELOG: GUI: Make checkboxes self-labelling

Context: Fixes an issue?

Fixes: #2723

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

Yes, User Manual and Server Manual screen shot updates are needed. This affects translations.

Status of this Pull Request

Needs review.
Depends on:

  • bugfix/remove-remaining-directory-server-mentions
  • bugfix/rpc-remaining-directory-server-mentions

What is missing until this pull request can be merged?

Needs 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

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Will try if I have time.

src/clientsettingsdlgbase.ui Show resolved Hide resolved
@pljones
Copy link
Collaborator Author

pljones commented Jan 21, 2023

Thanks. Will try if I have time.

Thanks.

@pljones
Copy link
Collaborator Author

pljones commented Jan 21, 2023

The "My Profile" tab allows the user to adjust the width of the window.
image
On Windows (GitHub build), using the drop down doesn't set itself to the correct width -- and I've no idea why.
image

The window shrinks width-ways too much
image

When it's stretching width-ways, some of the labels become unaligned - I did have a version with this fixed but I lost it somehow.
Aligned:
image
Unaligned:
Windows: image
Linux seems somewhat more prone to it (probably a skin-dependent issue as to how much it happens): image

@ann0see
Copy link
Member

ann0see commented Jan 21, 2023

The window shrinks width-ways too much

Not a regression I think. Someone requested it a while ago. But not sure why.

@ann0see
Copy link
Member

ann0see commented Jan 21, 2023

grafik

I think that's ok?

@pljones
Copy link
Collaborator Author

pljones commented Jan 22, 2023

The window shrinks width-ways too much

Not a regression I think. Someone requested it a while ago. But not sure why.

Yeah - the "form" version stopped it but broke the layout in other ways (i.e. stretch all fields equally).

src/serverdlg.cpp Outdated Show resolved Hide resolved
@gilgongo gilgongo added this to Triage in Tracking (old) via automation Jan 27, 2023
@gilgongo gilgongo moved this from Triage to Waiting on Team in Tracking (old) Jan 27, 2023
@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch from f5aecbf to 3d2b392 Compare February 17, 2023 18:48
@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch 2 times, most recently from 213fe51 to d47e94c Compare February 26, 2023 17:51
@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch 4 times, most recently from 305fc49 to 97032d7 Compare April 1, 2023 14:12
@pljones
Copy link
Collaborator Author

pljones commented Apr 5, 2023

I've got a new version of https://github.com/pljones/jamulus/tree/bugfix/%232723-self-label-checkboxes-forms2 available here:
https://github.com/pljones/jamulus/actions/runs/4616999421

The Windows version still has this annoying display error:
image
The window needs stretching about six pixels wider to get the dropdown to display correctly. I'd rather it "naturally" found the right width, as different font sizes (i.e. themes) may have different needs -- so I was wondering if this is a Qt 5 bug.

Can anyone - @jamulussoftware/maindevelopers particularly - take a look?

Also, @ann0see, @gilgongo or @emlynmac, could you check that build on a Mac, please. If it's okay (this is the Client Settings Dialog and Server Dialog changes) - including when resizing, please confirm here. I'll rebase and make it https://github.com/pljones/jamulus/tree/bugfix/%232723-self-label-checkboxes if it's okay, then we can progress this (once the directory changes are merged).

@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch 2 times, most recently from f7572ce to 2b8c51b Compare April 13, 2023 17:27
@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch 2 times, most recently from bff052a to 6c20966 Compare April 17, 2023 19:51
@pljones
Copy link
Collaborator Author

pljones commented Apr 17, 2023

In addition to the build here:

I could do with MacOS people checking both to see whether they work (profile, maximize window). I'd prefer to go with forms2.

(Of course, immediately outdated...)

@pljones pljones self-assigned this Apr 17, 2023
@pljones pljones added the needs documentation PRs requiring documentation changes or additions label Apr 17, 2023
@pljones pljones added this to the Release 3.10.0 milestone Apr 17, 2023
@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch from 6c20966 to b5445d1 Compare April 17, 2023 20:08
@gilgongo
Copy link
Member

In addition to the build here:

Just tried forms2 on Monterey 12.6.1 and it seems OK to me (profile, maximize window)

@gilgongo
Copy link
Member

Ah OK. BTW "Mixer Rows" would be nicer at about 25% of the length it is in that screenshot, but it's not a big problem.

@pljones
Copy link
Collaborator Author

pljones commented Apr 20, 2023

No, that's not the intended layout. That's why it was important to test on macOS explicitly as Linux and Windows do not show that behaviour.

Note, the request to check "maximize" applied to all settings tabs.

Four "important" files change here --

  • src/clientsettingsdlg.cpp and src/clientsettingsdlgbase.ui
    controlling the client settings UI. No other UI changes or code touched in the client but all three settings tabs affected.
  • src/serverdlg.cpp and src/serverdlgbase.ui
    controlling the server UI (no other server changes).

So those areas could do with review for rendering style on all platforms (ideally old versus new, various screen resolutions and layouts).

@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch from 6fa64cd to b5445d1 Compare April 21, 2023 20:22
@pljones
Copy link
Collaborator Author

pljones commented Apr 21, 2023

OK, reverted the one with the macOS breakage.

@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch from b5445d1 to b39f93a Compare April 23, 2023 08:52
@ann0see
Copy link
Member

ann0see commented Apr 26, 2023

So @pljones do you think the macOS build is ok with your latest changes?

@pljones
Copy link
Collaborator Author

pljones commented Apr 27, 2023

So @pljones do you think the macOS build is ok with your latest changes?

I've not had time over the last few days to keep up. Hopefully the current build on this branch is okay again.

@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch 2 times, most recently from f5d4591 to 78960f9 Compare May 1, 2023 16:03
@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch from 78960f9 to 6d1172c Compare May 10, 2023 17:04
@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch 3 times, most recently from 0ff51b6 to aee6bbe Compare May 22, 2023 18:41
@pljones pljones force-pushed the bugfix/#2723-self-label-checkboxes branch from aee6bbe to a0df8cd Compare May 31, 2023 13:08
@ann0see
Copy link
Member

ann0see commented May 31, 2023

I will - hopefully - have access to a real Mac in the next weeks but no guarantees.

@pljones
Copy link
Collaborator Author

pljones commented Jun 2, 2023

OK, it builds and runs on Linux (6.5.1). One tiny glitch (likely 6.5.1 related) - opening settings, the "My Profile" tab didn't show until I switched tabs. Very odd...

@pljones
Copy link
Collaborator Author

pljones commented Jun 3, 2023

OK, it builds and runs on Linux (6.5.1). One tiny glitch (likely 6.5.1 related) - opening settings, the "My Profile" tab didn't show until I switched tabs. Very odd...

Confirmed on main -
Check settings:
image
No "My Profile tab"
image
Switch to "Advanced Setup" and it appears:
image
Once it's appeared, it stays for the duration of the run. Checking the checkbox multiple times doesn't fix it. Using the "Settings" menu to go straight to "My Profile" or using Ctrl-P both work okay.

So that'll need picking up under the move to Qt6.5.1...


And confirmed Qt5.15.2 doesn't show the strangeness.

I'm beginning to think Qt6 isn't ready for prime time.

@ann0see
Copy link
Member

ann0see commented Jun 3, 2023

Screenshot my profile

This is the current artifact from this PR - and I believe it's ok.

Comment on lines +40 to 43
// always start on the main tab
tabWidget->setCurrentIndex ( 0 );

// set window title
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the Qt 6.5 bug also occur if this isn't set?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's src/serverdlg.cpp, not related to the client settings "My Profile" weirdness.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh. Yes. It's a different file - so that's true.

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on the basis that this works on the CURRENT Qt version.

@pljones pljones merged commit d7465f5 into jamulussoftware:main Jun 4, 2023
10 checks passed
Tracking (old) automation moved this from Waiting on Team to Done Jun 4, 2023
@pljones
Copy link
Collaborator Author

pljones commented Jun 4, 2023

Merging - we can pick up any reported problems later. Hopefully it'll be okay.

@ann0see
Copy link
Member

ann0see commented Jul 24, 2023

@pljones does this still need documentation? If no, please drop the needs documentation label.

@pljones pljones removed the needs documentation PRs requiring documentation changes or additions label Jul 25, 2023
@pljones pljones removed this from Done in Tracking (old) Jul 28, 2023
@gilgongo gilgongo mentioned this pull request Aug 8, 2023
57 tasks
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.

Checkbox controls should be "self-labelling"
4 participants