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

Add profile checkbox to enable/disable tooltips #3446

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

softins
Copy link
Member

@softins softins commented Dec 24, 2024

Short description of changes

Adds a checkbox to the My Profile dialog to enable tooltips to be enabled or disabled. The setting is stored in the .ini file. A Qt event filter consumes the tooltip event if tooltips are disabled, preventing the tooltip from being shown.

CHANGELOG: Client: add a checkbox to My Profile to enable/disable tooltips.

Context: Fixes an issue?

The PR #3252 adds additional tooltips to the Connect dialog, some of which are quite wordy. While these tooltips will be useful to new or occasional users of Jamulus, they can quickly become annoying to experienced users who have no need of the wordy reminder, particularly within the server list. Allowing tooltips to be disabled caters for both types of user.

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

Any Wiki page that has a screenshot and description of the the My Profile page should be updated.

Status of this Pull Request

Tested and working.

What is missing until this pull request can be merged?

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

@softins softins mentioned this pull request Dec 24, 2024
5 tasks
@softins softins added this to the Release 3.12.0 milestone Dec 25, 2024
@softins softins self-assigned this Dec 25, 2024
@ann0see
Copy link
Member

ann0see commented Dec 25, 2024

Thanks. Is there a reason why it's not an advanced setting?
I understand that it's UI related and most UI related settings are in my profile. However advanced users would have this disabled, so it would be an advanced setting.

@pljones
Copy link
Collaborator

pljones commented Dec 26, 2024

Could you set the project, please, so the PR gets tracked.

@pljones
Copy link
Collaborator

pljones commented Dec 26, 2024

Ta.

Does this disable tooltips - such as used in the mixer view for profile information popups - everywhere?

@softins
Copy link
Member Author

softins commented Dec 26, 2024

Does this disable tooltips - such as used in the mixer view for profile information popups - everywhere?

It certainly does so in both the settings dialog and the connect dialog. I didn't check the mixer, as I forgot there were tooltips for profile information. I can do so when I'm next at the computer. I think it probably does. If that's a problem, maybe we can add another check to the filter.

@softins
Copy link
Member Author

softins commented Dec 26, 2024

Thanks. Is there a reason why it's not an advanced setting?
I understand that it's UI related and most UI related settings are in my profile. However advanced users would have this disabled, so it would be an advanced setting.

No particular reason, other than it looked a natural place to put it. I guess the same question could be raised about the Audio Alerts checkbox?

@softins
Copy link
Member Author

softins commented Dec 26, 2024

Could you set the project, please, so the PR gets tracked.

It appears to be set already? I'm not entirely clear how the project feature works.

@@ -397,6 +397,11 @@ CClientSettingsDlg::CClientSettingsDlg ( CClient* pNCliP, CClientSettings* pNSet
"A second sound device may be required to hear the alerts." ) );
chbAudioAlerts->setAccessibleName ( tr ( "Audio Alerts check box" ) );

// show tooltips
chbShowToolTips->setWhatsThis ( "<b>" + tr ( "Show ToolTips" ) + ":</b> " +
Copy link
Member

Choose a reason for hiding this comment

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

I believe it should be "Tooltips" by the style guide.
@gilgongo ?

Copy link
Member

Choose a reason for hiding this comment

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

The Guide says "Nouns use lower case unless they have a formal definition in this style guide". So it looks like if we're to be consistent with "check box" (for example) we might need to have it as "Show tool tips".

As an aside, we also use capitals to refer to things unique or in some way special to Jamulus, and not only terms in the Guide (hence "Audio Alerts" for example). Might add that to the Guide in fact. I like to think our slightly unusual use of capitals is inherited from the German origins of Jamulus :-)

@softins softins force-pushed the tooltips-enable-disable branch from 67adbc9 to 734e8e0 Compare December 31, 2024 17:14
@softins
Copy link
Member Author

softins commented Dec 31, 2024

Does this disable tooltips - such as used in the mixer view for profile information popups - everywhere?

Yes, just tested, and with tooltips disabled, the musician profile popups are not displayed. Do we need to test for this and force just them to be enabled, or provide a separate setting, or leave them to be the same as other tooltips?

@pljones
Copy link
Collaborator

pljones commented Jan 1, 2025

I'd go with

force just them to be enabled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting on Team
Development

Successfully merging this pull request may close these issues.

4 participants