remove WTrackText class, use WTrackProperty or WLabel #12004

ronso0 · 2023-09-20T08:40:05Z

reduce code duplication / maintenance

Update
First, I made WTrackText inherit from WTrackProperty (diff was only the setup()), then realized that the Text widget was actually a WLabel with a fixed text and the ability to read a Group to show 'track info' #12004 (comment)

Now, WTrackText is removed entirely. When parsing a Text widget node LegacySkinParser decides right away whether to construct a WLabel or a WTrackProperty:

if a Group is defined (or Channel, legacy) use WTrackProperty (ignore Text node)
- invalid group: empty
- valid group:
  - Property defined: show that (empty if property is invalid)
  - no Property node: show track info
if Text is defined simply use WLabel

Outcome:
Both Text and TrackProperty work as before, just without the code duplication of WTrackText.

src/widget/wtracktext.cpp

Swiftb0y · 2023-09-20T12:53:59Z

src/widget/wtracktext.cpp

-        m_pTrackMenu->popup(event->globalPos());
-    }
+void WTrackText::setup(const QDomNode& node, const SkinContext& context) {
+    WLabel::setup(node, context);


as rightfully pointed out by clazy.

Suggested change

WLabel::setup(node, context);

WTrackProperty::setup(node, context);

nope, false positive: we do exactly not want that setup which parses the widget context for Property node since WTrackText is explicitly trackInfo not some specific tag.

How can we let clazy know about it?

See https://github.com/KDE/clazy/blob/master/README.md#reducing-warning-noise
Feel free to reformulate the comment.

Suggested change

WLabel::setup(node, context);

// we do exactly not want that setup which parses the widget context

// for Property node since WTrackText is explicitly trackInfo not some specific tag.

WLabel::setup(node, context); // clazy:exclude=skipped-base-method

Thanks! Was on mobile so looking this up was rather cumbersome.
Fixed.

I understand. In that case the base class would handle track loading, dnd & context menu events, and it would be of no practical use (no text is displayed, much like WTrackWidgetGroup).
The derived classes would only need to define setup, updateLabel and mouseDoubleClickEvent.

Actually, with this PR, we could deprecate WTrackText (just migrate to WTrackProperty in the official skins) and the outcome would be the same, though I didn't consider that since in the first place because WTrackText is probably used in custom skins.

If I would have thought of that, I'd also had been obvious that we can drop WTrackText and use WTrackProperty with just a few changes in legacyskinloader 🤦‍♂️
See last commit.

No, that would have been too easy.
WtrackText is just the Text widget and it's designed for two scenarios:

if there's a Text defined that string is displayed (basically the same as WLabel)

if there's a Group defined it shows "track info" (artist &/ title / file name) (and ignores Text, if defined)

I think my proposal from above can be implemented in legacyskinparser:

if a Group (or Channel, legacy) is defined use WTrackProperty (ignore Text node)

invalid group: empty

valid group:

Property defined: show that (empty if property is invalid)

no Property node: show track info

if Text is defined simply use WLabel

Outcome:
WTrackProperty + WLabel, drop WTrackText class

I'll take a look. If the changes are too cumbersome I propose to merge this PR as a first step.

Sure... I don't understand all of what you're saying because I'm not familiar enough with the skin system so I can't really form a well informed opinion.
This PR doesn't worsen the code in anyway, so sure we can merge now and do the rest later.

Ha, done!

Test for skin regression without the skin migration commit f29580c

Testing without f29580c:

all knob labels and fixed deck labels should be okay

the only place where Text was used from 'track info' are all preview decks and samplers Shade

src/widget/wtracktext.h

ronso0 · 2023-09-29T11:55:32Z

I updated the PR description.
Let me know if you think we need more comments or skin warnings to point out the migration (I doubt it).

ronso0 · 2023-09-29T12:16:15Z

I'm not sufficiently familiar with QMetaProperty, I need some help with moving the the property checks to setup, i.e. remove
https://github.com/mixxxdj/mixxx/pull/12004/files#diff-013766ebc9381f99ed2ced22f2dc4d51f48f143ab8f4a251a065304b0599e8a5R99-R101
and do all checks here
https://github.com/mixxxdj/mixxx/pull/12004/files#diff-013766ebc9381f99ed2ced22f2dc4d51f48f143ab8f4a251a065304b0599e8a5R59
Is this feasible? Is it recommended, or should we simply leave as it is?

daschuer · 2023-09-29T20:38:25Z

Yes it should be possible. Take a look here:
https://github.com/qt/qtbase/blob/94df3f8d6bfb40c9339256a93e47a495caff152c/src/corelib/kernel/qobject.cpp#L4249

Not sure if everything is accessible though but the code in may store the ID WTrackProperty::setup() and than use it in WTrackProperty::updateLabel via (Untested) :

const auto *pMetaObject = m_pCurrentTrack->metaObject();
if (pMetaObject) {
    QVariant propertyValue =  pMetaObject->property(m_propertyId).read(m_pCurrentTrack); 
}

daschuer

The code looks and works good, by the way. Just give a hint when I should press merge.

ronso0 · 2023-09-29T21:05:10Z

Thanks. I'm (briefly) looking into the property topic right now. Either I find a solution with your hints in the next hour, or not.

ronso0 · 2023-09-29T22:03:36Z

I'll give up regarding consolidating the property checks. What I had in mind was doing the QVariant::isvalid and QVariant::canConvert<QString> checks in the setup, not every time the label is updated. Though, canConvert is not static and I didn't figure a quick way to do the check without too much hazzle. If anyone wants to try it, go ahead.

Ready to be merged.
Thanks for your reviews!

ronso0 · 2023-09-29T22:04:59Z

Btw I'd squash the first two commits if you agree.

daschuer · 2023-09-30T04:03:55Z

Yes, go ahead.

ronso0 · 2023-10-24T09:35:15Z

Done, ready for merge.

…operty

daschuer · 2023-12-27T12:08:47Z

Thank you.

ronso0 · 2023-12-27T21:15:28Z

Oh, I forgot to remove the now unused files.
I did so now, and also merged 2.4 into main.

github-actions bot added the ui label Sep 20, 2023

ronso0 force-pushed the wtracktext-subclass branch from f5a5f2b to dec47ea Compare September 20, 2023 08:41

ronso0 marked this pull request as draft September 20, 2023 10:02

ronso0 force-pushed the wtracktext-subclass branch from dec47ea to a68fb79 Compare September 20, 2023 10:08

ronso0 marked this pull request as ready for review September 20, 2023 10:08

daschuer reviewed Sep 20, 2023

View reviewed changes

src/widget/wtracktext.cpp Outdated Show resolved Hide resolved

Swiftb0y reviewed Sep 20, 2023

View reviewed changes

ronso0 force-pushed the wtracktext-subclass branch from a68fb79 to ceb7754 Compare September 21, 2023 07:56

Swiftb0y reviewed Sep 21, 2023

View reviewed changes

src/widget/wtracktext.h Outdated Show resolved Hide resolved

ronso0 force-pushed the wtracktext-subclass branch from ceb7754 to d1ba96d Compare September 21, 2023 09:19

github-actions bot added build skins labels Sep 28, 2023

ronso0 changed the title ~~make WTracktext a WTrackProperty subclass~~ remove WTrackText class, use WTrackProperty or WLabel Sep 28, 2023

ronso0 requested a review from daschuer September 28, 2023 23:26

daschuer approved these changes Sep 29, 2023

View reviewed changes

ronso0 force-pushed the wtracktext-subclass branch from 53413b6 to c18e41f Compare September 30, 2023 12:52

ronso0 force-pushed the wtracktext-subclass branch from c18e41f to e3e547b Compare December 25, 2023 14:39

ronso0 added 2 commits December 25, 2023 19:45

remove WTrackText class, adjust skin parser to use WLabel or WTrackPr…

e11373a

…operty

skins: migrate Text -> TrackProperty, remove empty Text nodes

c4b6161

ronso0 force-pushed the wtracktext-subclass branch from e3e547b to c4b6161 Compare December 25, 2023 19:26

ronso0 mentioned this pull request Dec 25, 2023

WTrackProperty: show simple editor on delayed double-click #11755

Merged

5 tasks

daschuer merged commit d96d929 into mixxxdj:2.4 Dec 27, 2023
13 checks passed

ronso0 deleted the wtracktext-subclass branch December 27, 2023 12:44

ronso0 mentioned this pull request Jan 23, 2024

No title on Preview Deck using LateNight Classic #12629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove WTrackText class, use WTrackProperty or WLabel #12004

remove WTrackText class, use WTrackProperty or WLabel #12004

ronso0 commented Sep 20, 2023 •

edited

Loading

Swiftb0y Sep 20, 2023

ronso0 Sep 20, 2023

ronso0 Sep 20, 2023

Swiftb0y Sep 20, 2023

ronso0 Sep 21, 2023

ronso0 Sep 28, 2023

ronso0 Sep 28, 2023 •

edited

Loading

Swiftb0y Sep 28, 2023

ronso0 Sep 28, 2023

ronso0 Sep 28, 2023

ronso0 commented Sep 29, 2023

ronso0 commented Sep 29, 2023

daschuer commented Sep 29, 2023

daschuer left a comment •

edited

Loading

ronso0 commented Sep 29, 2023

ronso0 commented Sep 29, 2023

ronso0 commented Sep 29, 2023

daschuer commented Sep 30, 2023

ronso0 commented Oct 24, 2023

daschuer commented Dec 27, 2023

ronso0 commented Dec 27, 2023

	WLabel::setup(node, context);
	WTrackProperty::setup(node, context);

-    WLabel::setup(node, context);
+    // we do exactly not want that setup which parses the widget context
+    // for Property node since WTrackText is explicitly trackInfo not some specific tag.
+    WLabel::setup(node, context); // clazy:exclude=skipped-base-method

remove WTrackText class, use WTrackProperty or WLabel #12004

remove WTrackText class, use WTrackProperty or WLabel #12004

Conversation

ronso0 commented Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Sep 29, 2023

ronso0 commented Sep 29, 2023

daschuer commented Sep 29, 2023

daschuer left a comment • edited Loading

Choose a reason for hiding this comment

ronso0 commented Sep 29, 2023

ronso0 commented Sep 29, 2023

ronso0 commented Sep 29, 2023

daschuer commented Sep 30, 2023

ronso0 commented Oct 24, 2023

daschuer commented Dec 27, 2023

ronso0 commented Dec 27, 2023

ronso0 commented Sep 20, 2023 •

edited

Loading

ronso0 Sep 28, 2023 •

edited

Loading

daschuer left a comment •

edited

Loading