(fix) apply Library date format, fix 'dateadded' filter - #16047
Conversation
07f7c91 to
28e39ca
Compare
|
I went through the fixups, they look good to me.
|
daschuer
left a comment
There was a problem hiding this comment.
I cannot confirm that "Apply" works. My test:
- Show tracks view with the "date added" column
- Date is shown as 3/1/26
- Select ISO in preference
- Nothing happens (unlike the text font for example)
- Closing the preferences via Cancel
- Date format changes to ISO.
My expectation is that it returns to the system default.
Apply does not change the date format.
It looks like that closing the dialog allays applies, regardless which button is used to close the dialog.
|
Okay, the wording "apply" was not adequate: previously the format was not saved at all, now it's saved on every change.
That is a repaint issue: on font change a repaint seems to be triggered immediately, for date format change you need to hover the library for that. |
|
Apply/Cancel/OK works now. The mass refresh is missing on Apply. Hovering does the refresh. |
|
Well, setting a font on a widget emits a Here, I find 1) more interesting tbh, but I don't know if 1 or 2 are worth the effort. Btw I think we don't need the instant update on change (without Apply) as well as reset on Cancel like for the font -- the date preview string is sufficient IMO. |
This is cumbersome, too, as it requires including BaseTrackTableModel (huge) in WLibraryTableView. |
|
This is my test with the broadcaster as proposed above. Works just fine! Note: all views receive the dataChanged() signal and while all seem to do some prep work (layout may have changed, doesn't apply here) only the visible view is repainted. still think it's too much just for the date update but I learned something in this experiment |
17f4dca to
028c54c
Compare
|
I simplified the broadcaster (is now a Mixxx Singleton) and it works well. Again, if you consider this too much for the update purpose, I'll remove it. |
028c54c to
523df6f
Compare
19cabc0 to
4cd00cd
Compare
|
ping @mixxxdj/developers The 'instant update' thingy is debatable of course, am okay to merge without that. |
ywwg
left a comment
There was a problem hiding this comment.
code looks fine to me, did not hand-test
|
ding dong can we merge the fix? with or without instant update, idc |
|
CI is failing! |
4cd00cd to
80e05b7
Compare
|
Ah, the fixup commit. Squashed now. |
| } | ||
|
|
||
| int dateIndex = comboBox_dateFormat->findData(QVariant::fromValue(preset)); | ||
| qWarning() << "-> idx:" << dateIndex; |
There was a problem hiding this comment.
these warnings look more like debug comments. The mixxx warning log is already very noisy, try to be efficient and minimal in your logging output, with as few lines as possible and identifying information in the line about what's being logged. Probably these can all be totally removed.
There was a problem hiding this comment.
that's from the TRACE commit which is of course removed before merge.
The other commits look good?
Did you test it?
eb44d6e to
7362c2c
Compare
|
ping |
|
Here is the output |
|
Thanks, does everything work as expected? |
|
The local time format still isn't working; it only displays YYYYY-MM-DD |
|
I need more details: and the what happens when you select another format in the preferences, eg. Regional Long? |
|
Okay, now I've tested it more extensively. Regional Short -> searchbar works -> tracks view: d/M/yy Native (System Default) -> searchbar doesn't work -> tracks view: dd.MM.yy Edit: Native (System Default) -> searchbar ISO format works -> tracks view: dd.MM.yy Is this version 2.7? Will this be fixed in 2.5 or 2.6? |
|
So all options are working now? |
Yea, this is a fix for a 2.7 feature (main = 2.7-alpha) |
I find the different formats of the native (system default) searchbar and library confusing.
Have you done anything yet? Should I test something? |
|
I take that as a yes. What exactly is confusing about the date options?
Not yet, will let you know.
oh, that doesn't work. |
Native (System Default) -> In the search bar, the format yyyy-MM-dd works, whereas in the tracks view it is dd.MM.yy
I've shortened it a bit; I hope that's enough |
ISO is always working since that is the format used by the "New" filter in the Analyze view. And here it is, search is run when the Analyze view is created:
I assume that is you tying "01.02.20"
Afterwards we assume that any date before 2000 is based on 1900, so we add 100 years. @andynahtod I pushed a fix, please try again. |
7362c2c to
1d20895
Compare
|
@ronso0 I can't build it |
|
This PR doesn't touch any delegates, I suggest you force-pull again and retry. |
There was a problem hiding this comment.
Pull request overview
Follow-up to the library date-format feature so the configured format is loaded/applied consistently across library models and the dateadded search filter can parse non-ISO dates.
Changes:
- Adds a date-format change broadcaster and hooks
BaseTrackTableModelup to repaint date-based columns when the format changes. - Refactors Library preferences date-format state handling so the selected format is loaded from config and applied on update/apply.
- Extends
DateAddedFilterNodeto try the configured library format before falling back to locale parsing, and updates test/bootstrap wiring for the new singleton.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/librarytest.cpp |
Creates/destroys the new date-format broadcaster in library-oriented tests. |
src/preferences/dialog/dlgpreflibrary.h |
Adjusts the preview helper signature and stores current date-format state. |
src/preferences/dialog/dlgpreflibrary.cpp |
Reworks date-format loading, preview updates, and apply-time persistence in Library preferences. |
src/library/searchquery.cpp |
Updates dateadded parsing to try the configured library date format. |
src/library/library.cpp |
Creates and destroys the broadcaster during Library lifetime. |
src/library/dateformatbroadcaster.h |
Declares the new singleton signal broadcaster for date-format changes. |
src/library/dateformatbroadcaster.cpp |
Defines the broadcaster implementation. |
src/library/basetracktablemodel.h |
Exposes the active date format and declares a repaint slot for date columns. |
src/library/basetracktablemodel.cpp |
Emits date-format change notifications and refreshes date columns when the format changes. |
CMakeLists.txt |
Adds the new broadcaster source file to the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try user date format set in library preferences | ||
| const QString dateFormat = BaseTrackTableModel::dateFormat(); | ||
| qWarning() << "--> invalid, try Lib format" << dateFormat; | ||
| date = QDate::fromString(dateStr, dateFormat); |
| // Try user date format set in library preferences | ||
| const QString dateFormat = BaseTrackTableModel::dateFormat(); | ||
| qWarning() << "--> invalid, try Lib format" << dateFormat; | ||
| date = QDate::fromString(dateStr, dateFormat); | ||
| } | ||
|
|
||
| if (!date.isValid()) { | ||
| qWarning() << "--> invalid, try locale format" | ||
| << QLocale().dateFormat(QLocale::ShortFormat); | ||
| // Maybe custom user format is too esoteric, or user picked | ||
| // their locale's format. | ||
| // Fall back to locale-specific short format | ||
| #if QT_VERSION < QT_VERSION_CHECK(6, 7, 0) | ||
| // If the year component has only two digits Qt assumes the base year is 1900. | ||
| date = QLocale().toDate(dateStr, QLocale::ShortFormat); | ||
| // The Mixxx project was started in 2001 :) | ||
| if (date.year() < 2000) { | ||
| date = date.addYears(100); | ||
| } | ||
| #else | ||
| date = QLocale().toDate(dateStr, QLocale::ShortFormat, 20); | ||
| // With Qt 6.7+ we need to specify the base year. | ||
| date = QLocale().toDate(dateStr, QLocale::ShortFormat, 2001); |
There was a problem hiding this comment.
Indeed, there is NO date filter test. And I'm not motivated to write one atm.
Let's stick to manual testing (and Copilot) whenever the date filter is touched.
There was a problem hiding this comment.
And I have no clue how to involve BaseTrackTableModel into the test 🤷♂️
1d20895 to
d2e570e
Compare
Everything works fine. |
However, this doesn't work in the Stable version; there, all tracks are always displayed—regardless of whether a filter is applied or not. |
Great, thanks for testing!
True, I'd use this too. |
|
@mixxxdj/developers @ywwg already approved this, can we please merge this regression fix? |
Follow-up for #15898