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

(fix) Multi-track 'Track Properties': accept entered text directly (clearing not required) #13631

Open
wants to merge 3 commits into
base: 2.5
Choose a base branch
from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Sep 6, 2024

Something I overlooked earlier, it seems:
previously we had to explicitly clear a tag before a manually entered text would be accepted as new text.

background: editors with multiple values have a placeholder text <various>. It is removed when another item is selected (Clear or text item).
On Apply this placeholder is checked to detect whether a multi-value field has been edited.

Now the placeholder is also cleared when the editingFinished() signal is fired (Return, Esc, focus out, checks for valid text).
For the comment editor this signal doesn't exist. There is textChanged() but this is fired for each change (character add/remove), which is not ideal. Instead we catch the focusOut event of the comment editor and check whether the current text is 'new'.

Small fix:
Don't use QString::trimmed() to be consistent with the track table's inline editor.
Only trailing whitespaces are removed, which allows to clear tags by entering a single space.

@ronso0 ronso0 added this to the 2.5.0 milestone Sep 6, 2024
@ronso0 ronso0 changed the title (fix) Multi-line editor: accept entered text directly (clearing not required) (fix) Multi-track editor: accept entered text directly (clearing not required) Sep 21, 2024
@ronso0 ronso0 changed the title (fix) Multi-track editor: accept entered text directly (clearing not required) (fix) Multi-track 'Track Properties': accept entered text directly (clearing not required) Oct 26, 2024
Copy link
Contributor

@cr7pt0gr4ph7 cr7pt0gr4ph7 left a comment

Choose a reason for hiding this comment

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

LGTM, except for the "trim trailing whitespace" code that should really be in an utility function (and should probably handle other types of (white)space instead of just the " ")

Comment on lines 62 to 67
// Remove trailing whitespaces. Keep Leading whitespaces to be consistent
// with the track table's inline editor.
while (currVal.endsWith(' ')) {
currVal.chop(1);
}
return currVal;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be moved into an utility function mixxx::trimTrailingWhitespace(QString) -> QString in utils/string.h

void DlgTrackInfoMulti::commentTextChanged() {
if (!txtComment->placeholderText().isNull() &&
!txtComment->toPlainText().isEmpty()) {
// The comboox has multiple values and has not been cleared yet,
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nitpick: comboox -> combobox

// Let's clear the placeholder text so we know this is new text.
txtCommentBox->blockSignals(true);
txtComment->setPlaceholderText(QString());
// The Clear item is not needed anymore, remove.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: // The Clear item is not needed anymore, so remove it.

@@ -84,6 +84,14 @@ inline QString convertWCStringToQString(
return QString::fromWCharArray(wcs, static_cast<int>(wcsnlen_s(wcs, maxLen)));
}

inline QString removeTrailingWhitespaces(const QString& str) {
Copy link
Contributor

@cr7pt0gr4ph7 cr7pt0gr4ph7 Nov 2, 2024

Choose a reason for hiding this comment

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

Suggested change
inline QString removeTrailingWhitespaces(const QString& str) {
/// Remove trailing spaces (" ") from the specified string.
///
/// Currently only handles simple ASCII spaces (" "). and none of the additional
/// Unicode whitespace characters (https://en.wikipedia.org/wiki/Whitespace_character#Unicode).
inline QString removeTrailingWhitespaces(const QString& str) {
// TODO(XXX): Evaluate whether it makes sense to extend this to all characters within the "Whitespace" Unicode character class

Maybe it would make sense to handle other kinds of whitespace that one may encounter when pasting something from somewhere else? For example, non-breakable spaces.

Although, after checking the sources and against my expectations, QString::trimmed() also only handles the ASCII whitespace characters, and none of the UTF-8/Unicode Whitespace Characters. This is opposed to e.g. .NET String::Trim() which supports all Unicode whitespace characters.

So, probably a topic for a different PR/issue/discussion.

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

Successfully merging this pull request may close these issues.

2 participants