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

Clear up Highlight sound settings #4194

Merged
merged 16 commits into from
Dec 3, 2022

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Nov 27, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

As discussed in #1540, this clears up the highlight settings for sounds:

Without custom default sound

firefox_2022-11-27_18-14-59

Closes #1540.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@kornes
Copy link
Contributor

kornes commented Nov 27, 2022

suggestions:

  • icon on clear button looks off, makes whole button kinda stands out too much. I would opt to remove it, or leave just icon, but then some variation of bin 🗑 would fit better i think.
  • disable clear button when no custom sound is set
  • maybe change current filename-only label, into input field with full file path, where user could paste/inspect path without clicking Change button. "Chatterino Ping" could be placeholder then.

@8thony
Copy link
Contributor

8thony commented Nov 27, 2022

Things I noticed:

  • If you have set a custom sound in the table before and then click to change the sound but hit the cancel button in the file browser, a ticked checkbox stays in the table. (Is this checkbox even necessary?)
  • The table entry with no option to play a sound doesn't block the option to set a custom sound.

@Nerixyz
Copy link
Contributor Author

Nerixyz commented Nov 27, 2022

  • icon on clear button looks off, makes whole button kinda stands out too much. I would opt to remove it, or leave just icon, but then some variation of bin 🗑 would fit better i think.

Removed.

  • disable clear button when no custom sound is set

It's hidden now.

  • maybe change current filename-only label, into input field with full file path, where user could paste/inspect path without clicking Change button. "Chatterino Ping" could be placeholder then.

This doesn't really fit the other settings. A user can now click on the path to open a file.

  • If you have set a custom sound in the table before and then click to change the sound but hit the cancel button in the file browser, a ticked checkbox stays in the table. (Is this checkbox even necessary?)

I removed the checkbox since it wasn't used.

  • The table entry with no option to play a sound doesn't block the option to set a custom sound.

There were actually settings for the sound-urls. But they weren't used, so I removed them.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@8thony
Copy link
Contributor

8thony commented Dec 3, 2022

If you set a sound with a long file name in the "Custom Sound" column and restart the program, the "Pattern" column becomes relatively narrow (Tested with 40 characters, including file extension)

If a sound with a long file name is set as "default sound", the settings window becomes wider (Tested with 80 characters, including file extension)

If you try this yourself, then make sure not to test the points above at the same time

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Changing sounds doesn't update, it requires a restart

src/singletons/Settings.hpp Outdated Show resolved Hide resolved
src/messages/SharedMessageBuilder.cpp Outdated Show resolved Hide resolved
@Nerixyz Nerixyz force-pushed the fix/highlight-sound-settings branch 2 times, most recently from 848d7a4 to a9bb85f Compare December 3, 2022 12:45
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@Nerixyz Nerixyz force-pushed the fix/highlight-sound-settings branch from 048778d to b2b6560 Compare December 3, 2022 13:18
@Nerixyz
Copy link
Contributor Author

Nerixyz commented Dec 3, 2022

If you set a sound with a long file name in the "Custom Sound" column and restart the program, the "Pattern" column becomes relatively narrow (Tested with 40 characters, including file extension)

Should be twice the width now, though this should really be changed after #3690.

If a sound with a long file name is set as "default sound", the settings window becomes wider (Tested with 80 characters, including file extension)

Filenames are now truncated to 50 characters.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@8thony
Copy link
Contributor

8thony commented Dec 3, 2022

Should be twice the width now, though this should really be changed after #3690.

It doesn't change anything for me, but it seems to be a general issue that isn't unique to this settings page.

Filenames are now truncated to 50 characters.

It's okay. Maybe it shouldn't be limited to a certain number but to the current size of the window. So making it wider reveals more of the name. I'll let you decide what is better.

@pajlada pajlada changed the title refactor: Clear Up Confusing Highlight Sound Settings Clear up Highlight sound settings Dec 3, 2022
@pajlada pajlada merged commit 2aa8af4 into Chatterino:master Dec 3, 2022
@Nerixyz Nerixyz deleted the fix/highlight-sound-settings branch December 3, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlight sound settings are confusing
4 participants